Adding A Modal Component #1369
Adding A Modal Component #1369
Conversation
|
@argyleink @surma could y'all take a look at this whenever you're free? |
There was a problem hiding this comment.
Overall pretty good! Spacing works, sizes work, but the colors were a bit off the rails, so I've made a few suggestions to bring it back into the theme.
A couple other notes!
- I agree with others that dialog is a good choice here, it'll save a bunch of maintenance burden (like trying to trap focus, etc) and I dont think a majority of the users are on iOS making these transformations
- The labels should probably also be keyboard focusable. It's a bit strange that focus is "restored" but not really to where I came from, rather the nearest checkbox to where I came from. Also not sure how to invoke this from keyboard, tho it's clear how to dismiss it.
Cool idea here. Hope this review is helpful!
|
@argyleink That's a wayyy better design, thank you for the review! And yep, my plan is to give an optional prop to the Working on a refactor using the |
Caveats:
|
|
Switched from using Context/Provider to shared functions |
* Improvements to Modal UI * Modal is now keyboard accessible * Added new/better info, error and update icons
TS 4.8.3 onwards supports `showModal()`. Also bumped @types/node.
There was a problem hiding this comment.
This is heading in the right direction, but I think trying to make the modal a singleton is unnecessary.
One of the benefits of <dialog> is that it doesn't need to be placed out-of-context in the top of the DOM, as it pops into the top-layer when it opens. It feels like the implementation will become simpler if we lean into that ("use the platform" etc etc).
So, the usage of this component would be:
<Modal shown={bool} icon={iconJSX} title="Hello!">
Modal content goes here.
</Modal>We'd have many of those in our JSX, in context.
And when shown changes, the component handles the animated show and hide.
I guess at some point we'd also have a tooltip component that outputs an icon and a modal, and causes the modal to show when the icon is clicked.
|
Thinking about the helper component: <ModalHelpIcon title="Hello!">
Modal content goes here.
</ModalHelpIcon>So the above would output a ℹ️ icon (or whatever), and a That means we can just add |
|
Added a
|
|
fun additions! I've got a 2 overall comments 🙂
|
|
| <ModalHint modalTitle="mozJPEG - quality" content={sampleContent()}> | ||
| Quality: | ||
| </ModalHint> |
There was a problem hiding this comment.
I think it makes more sense for the content of the modal to be the child of <ModalHint>, and the 'label' to be a prop. The label could probably just be a string too.
There was a problem hiding this comment.
Hmm I suppose that makes more sense. My thought process was we can just wrap existing text nodes (like for the custom input elements) in the ModalHint and get the modal functionality without much refactoring - but in hindsight that's a cop-out.
There was a problem hiding this comment.
It works both ways, and in this case it isn't clear-cut which thing is naturally the "children". But "content" sounds like just another name for "children", whereas the actual child is the label. That, and I usually lean towards having the larger, more tree-like content as the child.
But yeah, it isn't clear-cut.
| // animate modal::backdrop | ||
| // some browsers don't support ::backdrop, catch those errors | ||
| try { | ||
| animateTo(this.dialogElement, [{ opacity: 0 }], { |
There was a problem hiding this comment.
| animateTo(this.dialogElement, [{ opacity: 0 }], { | |
| animateTo(this.dialogElement, { opacity: 0 }, { |
Minifying icons using SVGOMG Co-authored-by: Jake Archibald <[email protected]>
ModalHint now takes the modal content as its children, and the optional text through props
|
@argyleink @kosamari @surma UX question: Right now the whole label becomes an activator for the modal (see "Auto sample chroma" in the preview). My hunch is that only the icon should activate the modal, and the rest of the label should remain as an activator for the checkbox. Can I get a second opinion? |
| outline: var(--medium-light-gray); /* Overwrite default black outline */ | ||
| } | ||
|
|
||
| /* Don't show focus ring on mobile */ |
There was a problem hiding this comment.
| /* Don't show focus ring on mobile */ | |
| /* Only show the focus ring on keyboard interaction */ |
| interface Props { | ||
| icon: VNode; | ||
| title: string; | ||
| content: VNode; |
There was a problem hiding this comment.
I think we decided that the content of the model should be the children of the modal?
| onClick={(event) => { | ||
| // Prevent clicks from leaking out of the dialog | ||
| event.preventDefault(); | ||
| event.stopImmediatePropagation(); |
There was a problem hiding this comment.
I don't think it needs to stop immediate propogation?
| } | ||
|
|
||
| private _hideModal() { | ||
| if (!this.dialogElement) throw Error('Modal missing / hidden'); |
There was a problem hiding this comment.
I think it's ok to assume dialogElement is there, since this component never presents a state where it isn't. Other instances of this can be skipped too.
|
|
||
| interface Props { | ||
| modalTitle: string; | ||
| text?: string; |
There was a problem hiding this comment.
Let's change this to "label" or something that better describes its usage.
| onClick={this.onclick} | ||
| title="Learn more" | ||
| > | ||
| <InfoIcon></InfoIcon> |
There was a problem hiding this comment.
| <InfoIcon></InfoIcon> | |
| <InfoIcon/> |
| // When the button is clicked, the event starts bubbling up | ||
| // which might cause unexpected behaviour | ||
| event.preventDefault(); | ||
| event.stopImmediatePropagation(); |
There was a problem hiding this comment.
I don't think stopImmediatePropagation is needed.
|
|
||
| private onclick = (event: Event) => { | ||
| if (!this.modalComponent) | ||
| throw new Error('ModalHint is missing a modalComponent'); |
There was a problem hiding this comment.
This can probably be skipped, as it's guaranteed to be there.
Confirm, label clicks are typically activators for an associated checkbox, plus this is also how Squoosh works today, using labels as activators. I agree with Jake, the icon should isolate the interaction to learn more from a modal, both because users expect labels to activate checkboxes on the web, but also because that's the UX that users currently expect from the label interactions. Same goes for the nested link in the mozJPEG quality modal, a link should be a link and a button to invoke a modal should be a button to invoke a modal. Button was a good element choice to wrap the icon, as this keeps it in keyboard tab flow 👍🏻 |
* Optimized SVG paths * Removed unnecessary attributes Co-authored-by: Jake Archibald <[email protected]>
This PR adds a custom Modal component so that Squoosh can communicate better with users (snackbars are SO last year).
While the snackbar popup will convey small and concise messages to users, the Modal can be used for displaying verbose information like:
It's keyboard accessible, has pre-formatted styles for different HTML elements, is responsive across the board and can be easily accessed by other components/elements.
I've tested the feature on the following devices:
DEMO
In the right-side options under the quality slider, click on any of the modal checkboxes. If you're using a keyboard, focus will be restored to that checkbox once the modal is closed.
Unnecessary PR Lore: This is complementary to PR #1366 where Surma rightly pointed out that the HTML hover title is inaccessible to mobile users. I initially planned to put small one-line descriptions for each codec param in the title attribute, then later add modal support with more detailed descriptions. But I figure if those one-liners will be overwritten anyway, might as well do them soon.