GoogleChromeLabs / squoosh

Make images smaller using best-in-class codecs, right in the browser.
https://squoosh.app
Apache License 2.0
21.33k stars 1.48k forks source link

Adding A Modal Component #1369

Open aryanpingle opened 1 year ago

aryanpingle commented 1 year ago

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:

  1. INFO mode - Explaining codecs and their parameters
  2. UPDATE mode - Informing users about new features
  3. ERROR mode - Expanding error messages on mobile, where users can't view the console error messages

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.

aryanpingle commented 1 year ago

@argyleink @surma could y'all take a look at this whenever you're free?

aryanpingle commented 1 year ago

@argyleink That's a wayyy better design, thank you for the review!

And yep, my plan is to give an optional prop to the Range / Checkbox / etc components that turns their labels into modal-activating keyboard-navigable buttons. So far this was just a test to see if focus can be returned to the calling checkbox, which it can.

Working on a refactor using the <dialog> element as we speak 👍

aryanpingle commented 1 year ago

Caveats:

aryanpingle commented 1 year ago

Switched from using Context/Provider to shared functions

jakearchibald commented 11 months ago

Thinking about the helper component:

<ModalHelpIcon title="Hello!">
  Modal content goes here.
</ModalHelpIcon>

So the above would output a ℹī¸ icon (or whatever), and a <Modal />. When the icon is clicked, it shows the modal, by toggling the shown prop passed to <Modal />.

That means we can just add <ModalHelpIcon /> where we want to details codecs etc, and that component takes care of the show/hide state.

aryanpingle commented 11 months ago

Added a ModalHint component that contains a Modal within itself and shows it when activated. Went with a subtle green accent color so that hints like in the image below stand out. @argyleink, would love your thoughts on this!

image

argyleink commented 11 months ago

fun additions!

I've got a 2 overall comments 🙂

  1. replace the green with var(--blue) (maybe) or var(--less-light-gray) (prolly), like you've used in the dialog header here. info icons shouldn't try and steal attention, unless hovered, you can spice them up then.
    Screenshot 2023-07-26 at 7 38 22 AM

  2. there's alignment issues, here's some screenshots Screenshot 2023-07-26 at 7 38 06 AM Screenshot 2023-07-26 at 7 38 33 AM

aryanpingle commented 11 months ago
jakearchibald commented 9 months ago

@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?

argyleink commented 9 months ago

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?

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 👍đŸģ