-
Notifications
You must be signed in to change notification settings - Fork 66
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
add a modal component #34
Conversation
Coverage decreased (-0.4%) to 98.077% when pulling a099b0e29b2bb0bb7ed26cb17c5d12a38bdcc1d9 on mroytman/modal-component into 35d2bf8 on master. |
Coverage decreased (-10.0%) to 88.525% when pulling d5dd2fed08dc437c9d7e63a62db3fba9b5a6cecd on mroytman/modal-component into 35d2bf8 on master. |
Coverage decreased (-9.08%) to 89.431% when pulling 3dad7f74bdbe5557f37d0c7f805d5d97c3d257e0 on mroytman/modal-component into 35d2bf8 on master. |
src/Modal/index.jsx
Outdated
display="Close" | ||
buttonType="secondary" | ||
onClick={this.close} | ||
inputRef={(input) => { this.buttons[this.props.buttons.length + 1] = input; }} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this okay to do? What if the user/developer wants buttons to appear dynamically? Will props change accordingly?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As a follow on, originally I had attempted to do something like inputRef={(input) => { this.buttons[this.buttons.length + 1] = input; }}. This was having un-intended side-effects, where the buttons array would grow and have null buttons. I think it's because the renderButtons function is executed asynchronously, and therefore, the close button reference may be inserted in to the array before the other buttons have been rendered, causing weird array structure.
Coverage decreased (-9.08%) to 89.431% when pulling 172003c6b45078f315fdacc616c2626236f15e36 on mroytman/modal-component into 35d2bf8 on master. |
Coverage decreased (-13.07%) to 85.441% when pulling 7471c2aaee808f3ed7cba56da2c98714903e39e2 on mroytman/modal-component into 35d2bf8 on master. |
let buttonElement = button; | ||
let buttonProps = button.props; | ||
|
||
if (button.type !== Button) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a little odd looking, but essentially I found that if a button is passed in as a prop as a Button object, the Button should be constructed with the Button's props. Otherwise, you can just create a Button object using the object that is passed in and has the shape of the Button propTypes.
src/Modal/index.jsx
Outdated
key={i} | ||
inputRef={(input) => { this.buttons[i + 1] = input; }} | ||
onKeyDown={this.handleKeyDown} | ||
/>); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had to end up creating each button passed in as a prop, even Button objects, because I had to add props for the proper functionality of the modal (i.e. key, inputRef, and onKeyDown).
src/Modal/index.jsx
Outdated
aria-label="Close" | ||
buttonType="light" | ||
onClick={this.close} | ||
inputRef={(input) => { this.buttons[0] = input; }} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I assume the X button will always be the first button to be in focus, so I hard-coded the index here.
src/Modal/index.jsx
Outdated
if (e.key === 'Escape') { | ||
this.close(); | ||
} else if (e.key === 'Tab') { | ||
e.preventDefault(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm using preventDefault here instead of at the top of the function so that the default enter/space functionality is maintained for button selection.
src/Modal/index.jsx
Outdated
document.removeEventListener('click', this.handleDocumentClick, true); | ||
} | ||
|
||
handleDocumentClick(e) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right now, this just ensures that the focusIndex is updated when someone clicks on a button and the focus moves to that button.
Coverage decreased (-13.07%) to 85.441% when pulling 0d59bec30074da3b8befa69b84e5fa2fce730f4d on mroytman/modal-component into 35d2bf8 on master. |
Coverage decreased (-12.3%) to 86.166% when pulling 9c8a6a0dfd97a978c15e1af57f323ba56c37beb1 on mroytman/modal-component into 35d2bf8 on master. |
src/Modal/Modal.stories.jsx
Outdated
]} | ||
/> | ||
)) | ||
.add('configurable title and body (creppy edition)', () => ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll fix this to be something more professional.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Professional, psh. Just change creppy
to creepy
and we're good.
|
||
componentDidUpdate(prevState) { | ||
if (this.state.open && !prevState.open) { | ||
this.xButton.focus(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm unsure about this. My intent was to put focus on the X button when the modal is opened via a button and not on the initial render.
@arizzitano I think this is good for a look when you have some time. TODO: Figure out the |
Coverage decreased (-13.0%) to 85.547% when pulling c57146dc3b64390ed4b3c3b6e6c1cf4ff5c7dc6f on mroytman/modal-component into 35d2bf8 on master. |
@@ -37,7 +37,7 @@ function Button(props) { | |||
); | |||
} | |||
|
|||
Button.propTypes = { | |||
export const buttonPropTypes = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is so we can reference the buttonPropTypes as part of the Modal PropTypes.
Coverage decreased (-0.07%) to 98.438% when pulling 440471cc9ff95375d4f57939b123096ea69b93ec on mroytman/modal-component into 35d2bf8 on master. |
src/Modal/Modal.stories.jsx
Outdated
<Button | ||
display="I am a button!" | ||
buttonType="light" | ||
onClick={() => console.log('I made a log!')} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ESLint complains about this log statement 💁.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably the best-practice way around this is to set up Storybook's actions
addon: https://github.com/storybooks/storybook/tree/master/addons/actions. Then instead of using a console statement (which typically we want to keep out of production code), your Button
's onClick
could just fire a Storybook action and it would show up right in the DOM.
If you feel like this is out of scope for this ticket (totally reasonable!) you can add a /* eslint-disable no-console */
to the top of the file with a TODO: remove comment.
title: PropTypes.oneOfType([PropTypes.string, PropTypes.element]).isRequired, | ||
body: PropTypes.oneOfType([PropTypes.string, PropTypes.element]).isRequired, | ||
buttons: PropTypes.arrayOf(PropTypes.oneOfType([ | ||
PropTypes.element, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TODO: This could use a custom validator.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm kinda split on this. On the one hand, there's out-of-the-box validators we could use to make this easy (https://www.npmjs.com/package/react-element-proptypes), but on the other hand HRH Abramov discourages typechecking that's this granular. I'm inclined to say just leave it as-is -- it'll make things easier if we end up adopting something like Flow instead of propTypes for typechecking.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good to me!
Coverage decreased (-0.07%) to 98.438% when pulling 8dce90f181d2ad86cc784a72b6b9293d23a32a12 on mroytman/modal-component into 35d2bf8 on master. |
I'm not sure what's going on with the Button coverage? |
Coverage decreased (-0.07%) to 98.438% when pulling 2b57fe2ef685d34ce85f92b87fd89c6cda164149 on mroytman/modal-component into 35d2bf8 on master. |
Coverage increased (+0.7%) to 99.219% when pulling 7930f8a5d64530815dd1709152eb696a6c87681a on mroytman/modal-component into 35d2bf8 on master. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@MichaelRoytman this looks rad! Great work on the implementation and test coverage (and bumping up total coverage -- 99.219%, awesome!). I made a few suggestions, the most crucial of which is around enabling overrides of the 'Close'
button text for i18n flexibility, rather than hardcoding it into the modal. Happy to talk this one through if you have questions. You might have started on this already but it could also use a README (see https://github.com/edx/paragon/blob/master/src/Table/README.md for an example).
src/Modal/Modal.stories.jsx
Outdated
} | ||
|
||
resetModalWrapperState() { | ||
this.setState({ open: false }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 this is also a good way to demo the onClose
function.
src/Modal/Modal.stories.jsx
Outdated
openModal() { | ||
this.setState({ | ||
open: true, | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Non-crucial suggestion: format this setState
call to be all on one line, the same as the one on line 25. Or change the other setState
call to look like this one -- as long as they're consistent.
|
||
.modal-open { | ||
display: block; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This guy could use a newline at the end. Sublime can do this for you automatically: go to Sublime Text > Preferences > Settings and then in the right hand panel ("Preferences.sublime-settings -- User) paste "ensure_newline_at_eof_on_save": true
before the closing }
at the end of the file. You may need to add a comma on the previous line too so it's valid JSON.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done; thanks for the helpful hint!
src/Modal/Modal.stories.jsx
Outdated
]} | ||
/> | ||
)) | ||
.add('configurable title and body (creppy edition)', () => ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Professional, psh. Just change creppy
to creepy
and we're good.
src/Modal/Modal.stories.jsx
Outdated
<Button | ||
display="I am a button!" | ||
buttonType="light" | ||
onClick={() => console.log('I made a log!')} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably the best-practice way around this is to set up Storybook's actions
addon: https://github.com/storybooks/storybook/tree/master/addons/actions. Then instead of using a console statement (which typically we want to keep out of production code), your Button
's onClick
could just fire a Storybook action and it would show up right in the DOM.
If you feel like this is out of scope for this ticket (totally reasonable!) you can add a /* eslint-disable no-console */
to the top of the file with a TODO: remove comment.
src/Modal/Modal.stories.jsx
Outdated
title="I am the modal!" | ||
body="I was invoked by a button!" | ||
/> | ||
)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just had an idea for another story: how about a modal where the body is an element containing other focusable elements (buttons, inputs, etc)? E.g.
body={(
<p>Enter your email address to receive our free SpamBook!</p>
<InputText
name="email"
label="Email Address"
/>
)}
That way we can show that sequential tab focus works for focusable elements within the modal, as well as the ones that are built into it. If you want to try an input, you can import InputText
or InputSelect
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's a good idea! Do you think adding, say, a button to the above example that would, theoretically, send you an e-mail with a book (theoretically, as I have no book to send), would be useful? I'd, of course, create some kind of story in which a button would actually be functional and wouldn't required sending e-mail. It would require another wrapper class, though, akin to ModalWrapper, and I fear that may be too complicated for the purposes of the storybook. I was thinking something along the lines of... select your favorite month, click a button, and I'll tell you what season it's in (maybe with a coordinating emoji), or something silly like that. Either way is fine with me!
); | ||
}); | ||
|
||
it('does nothing on invalid keystroke q', () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lol I love this. I kind of feel like "invalid keystroke q" is going to become the go-to for coverage fixes.
expect(buttons.last().matchesElement(document.activeElement)).toEqual(true); | ||
}); | ||
}); | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great coverage here 🙏
title: PropTypes.oneOfType([PropTypes.string, PropTypes.element]).isRequired, | ||
body: PropTypes.oneOfType([PropTypes.string, PropTypes.element]).isRequired, | ||
buttons: PropTypes.arrayOf(PropTypes.oneOfType([ | ||
PropTypes.element, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm kinda split on this. On the one hand, there's out-of-the-box validators we could use to make this easy (https://www.npmjs.com/package/react-element-proptypes), but on the other hand HRH Abramov discourages typechecking that's this granular. I'm inclined to say just leave it as-is -- it'll make things easier if we end up adopting something like Flow instead of propTypes for typechecking.
src/Modal/index.jsx
Outdated
PropTypes.element, | ||
PropTypes.shape(buttonPropTypes), | ||
])), | ||
onClose: PropTypes.func, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just realized something: we've hardcoded the string Close
in a couple places within this modal (lines 88 and 101), which isn't super flexible with regard to i18n. Could you add another optional prop closeText
with a default of 'Close'
, so consumers can override it if necessary? Probably would be good to add a story to demo this as well.
Coverage increased (+0.7%) to 99.24% when pulling 2dc96e9f40861e5755d2d0fbe181b4f8142581ed on mroytman/modal-component into 35d2bf8 on master. |
Coverage increased (+0.7%) to 99.24% when pulling 068865de4c8b9ed31c76021cc42e6901a7644527 on mroytman/modal-component into 35d2bf8 on master. |
@arizzitano This should be good for a re-review when you have a chance. I believe I've addressed all of your comments. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just one comment re: close button text -- patch that and then I think this is good to merge! 🚢
src/Modal/index.jsx
Outdated
<h5 className={styles['modal-title']} id={this.headerId}>{this.props.title}</h5> | ||
<Button | ||
display="×" | ||
aria-label="Close" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd change this aria-label to {this.props.closeText}
as well.
068865d
to
2aa5130
Compare
Coverage increased (+0.7%) to 99.24% when pulling 2aa51307f015b57d9849081cbab18ac8e0c7a16a on mroytman/modal-component into 35d2bf8 on master. |
2aa5130
to
cb7d004
Compare
Coverage decreased (-9.08%) to 89.431% when pulling 172003c6b45078f315fdacc616c2626236f15e36 on mroytman/modal-component into 35d2bf8 on master. |
No description provided.