Issue #44. Added React Modal! #206
Conversation
…nd not both / and ?
… Also cleaned up some of the design aspects and added responsiveness
fixed whitespace issues
| </table> | ||
| </div> | ||
| </div> | ||
| </ReactModal> |
There was a problem hiding this comment.
@greg-han Please indent properly so it's clear which tags are nested inside other tags. Use 2 spaces of indentation per level.
| @@ -21,13 +21,52 @@ | |||
| order: 1; | |||
| } | |||
|
|
|||
| .Modal{ | |||
| position: absolute; top: 120px; left: 60px; right: 60px; bottom: 60px; border: 1px solid rgb(204, 204, 204); background: rgb(255, 255, 255); overflow: auto; border-radius: 10px; outline: none; padding: 20px; | |||
There was a problem hiding this comment.
@greg-han One CSS rule per line, please 👍
|
Just fixed these issues. |
| <div className="column one-half"> | ||
| <table className="keyboard-mappings"> | ||
| <tbody> | ||
| j <tr> |
| </tbody> </table> | ||
| </div> | ||
| </div> | ||
| </ReactModal> |
There was a problem hiding this comment.
@greg-han Turn this into its own component and then use it here just like PursuanceMenu or the like is used -- like this: <PursuanceMenu /> (rather than putting all the code in-line).
There was a problem hiding this comment.
@greg-han Sorry for all the changes, but the React way of doing things is very different from the vanilla JS way.
There was a problem hiding this comment.
No problem, I appreciate the feedback!
Thanks for helping me improve.
Do you want the component on PursuancePage.js or in it's own js file such as Modal.js?
…ance page. Looks much cleaner and is the reacty way of doing it
… ref to open modal throughout page
| <RightPanel /> | ||
| </article> | ||
| <Router> | ||
| <div id="pursuance-page" className="content-ctn" onKeyDown={(event) => {this.child.handleKeyDown(event);}} tabIndex="0"> |
There was a problem hiding this comment.
onKeyDown={(event) => {this.child.handleKeyDown(event);}}
can be just
onKeyDown={(event) => this.child.handleKeyDown(event)}
or in this case -- even simpler -- just
onKeyDown={this.child.handleKeyDown}
| <div id="pursuance-page" className="content-ctn" onKeyDown={(event) => {this.child.handleKeyDown(event);}} tabIndex="0"> | ||
| <ShortcutsModal ref={instance => { this.child = instance; }} /> | ||
| <nav id="pursuance-nav"> | ||
| <PursuanceMenu /> |
There was a problem hiding this comment.
@greg-han Please indent 2 spaces per level at to show the structure of the HTML/JSX
|
|
||
| class PursuancePage extends Component { | ||
|
|
||
There was a problem hiding this comment.
Hey @elimisteve, I think you should totally leverage prettier on those tedious stuffs, I can think a PR later
| </Switch> | ||
| <RightPanel /> | ||
| </article> | ||
| <Router> |
There was a problem hiding this comment.
@greg-han This line shouldn't be different; check whitespace
| <RightPanel /> | ||
| </article> | ||
| <Router> | ||
| <div id="pursuance-page" className="content-ctn" onKeyDown={(event) => {this.child.handleKeyDown(event);}} tabIndex="0"> |
There was a problem hiding this comment.
Break down long lines like this to look more like:
<div
id="pursuance-page"
className="content-ctn"
onKeyDown={this.child.handleKeyDown}
...
/>
| <Route exact path="/pursuance/:pursuanceId/participants" component={ParticipantsView} /> | ||
| </Switch> | ||
| <RightPanel /> | ||
| </article> |
There was a problem hiding this comment.
@greg-han None of these lines should be different; check whitespace
| @@ -9,9 +9,10 @@ import DiscussView from './views/DiscussView'; | |||
| import ParticipantsView from './views/ParticipantsView'; | |||
| import RightPanel from '../RightPanel/RightPanel'; | |||
| import './PursuancePage.css'; | |||
| import ShortcutsModal from '../ShortcutsModal/ShortcutsModal'; | |||
There was a problem hiding this comment.
@greg-han Looks like you forgot to git add ShortcutsModal, yes?
Added a Modal!
Update Removed JS event listeners, and added an in-react-component listener.
Changed some of the styling and also added responsiveness to the media queries in pusruancepage.css.