fix: Remove autofocus on dialog text input, reduce category image size #395
Conversation
skanderm
commented
Apr 11, 2024
skanderm
commented
- Closes Keyboard pop up on mobile when reporting the sound #375
…ontal to save vertical space
| src={icon.src} | ||
| alt={`${title} icon`} | ||
| width={isDesktop ? 100 : 20} | ||
| height={isDesktop ? 100 : 20} |
There was a problem hiding this comment.
The new Next.js Image component has some responsive support but I had some difficulties making it work with the fill setting, so I reverted to the legacy Image component with media queries. The icon and the category text were on top of each other, and without fill you need to specify the width/height anyway
There was a problem hiding this comment.
What do you think about having multiple copies of each image and hiding/unhiding with display: none on different breakpoints?
I've been trying to stay away from useMediaQuery because it's JS based which has some downsides (biggest problem is layout shifts). In this case (inside a modal) it's not really an issue but don't want to encourage the pattern. Unless there's a better solution I don't know of?
There was a problem hiding this comment.
And yeah legacy Image isn't ideal, but we are using it elsewhere. It may get removed soon since it's been deprecated, so we'll be forced to figure it out at that point… but I guess it buys us some time
|
@Filadora Does this address the issue? Wondering whether the buttons being too small might affect UX for category selection. The simpler fix just removes |
|
@skanderm this is great! I think smaller buttons will simplify user experience since users won’t need to scroll and will be able to see all the buttons even when the keyboard is opened. I’ll make notes to get user feedback for this improvement during the next round of testing. And thank you for removing the autofocus (fixing the keyboard pop up). |
| src={icon.src} | ||
| alt={`${title} icon`} | ||
| width={isDesktop ? 100 : 20} | ||
| height={isDesktop ? 100 : 20} |
There was a problem hiding this comment.
What do you think about having multiple copies of each image and hiding/unhiding with display: none on different breakpoints?
I've been trying to stay away from useMediaQuery because it's JS based which has some downsides (biggest problem is layout shifts). In this case (inside a modal) it's not really an issue but don't want to encourage the pattern. Unless there's a better solution I don't know of?
| src={icon.src} | ||
| alt={`${title} icon`} | ||
| width={isDesktop ? 100 : 20} | ||
| height={isDesktop ? 100 : 20} |
There was a problem hiding this comment.
And yeah legacy Image isn't ideal, but we are using it elsewhere. It may get removed soon since it's been deprecated, so we'll be forced to figure it out at that point… but I guess it buys us some time
There was a problem hiding this comment.
Testing this out on narrow screens, just caught that it doesn't display nicely on widths less than ~370px or so.
I've been trying to keep it nice down to 320px, not sure if it's necessary but it's a good design system discussion. We should set some design guidelines.
Anyway @skanderm I'm approving, I think fixing the autofocus is more important than 320px wide screens (and it does scroll overflow so it's not unusable)
