-
-
Notifications
You must be signed in to change notification settings - Fork 2.9k
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
feat(web,a11y): replace IconButton with CircleIconButton #9153
Conversation
…/replace-icon-button
…/replace-icon-button
…/replace-icon-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.
FYI @Ethan13310, this will affect your new implementation of the responsive top navigation in #8776. For example, the mdiMenu
hamburger button in the top navigation will need to be moved over to a CircleIconButton
. Open to your thoughts on this too!
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.
Nice!
Could you please revert showing three dots button background while hovering albums and faces, it's really disruptive. In the addition it doesn't follow current theme, it's always shown in light theme. before: 20240502_081944.mp4PR: 20240502_081847.mp4 |
Hey @waclaw66, thanks for the feedback! The more prominent background on the icon was intentional, to give the button guaranteed color contrast in photos where the upper portion is brighter, such as an overexposed sky. To demonstrate, here's what the icon looks like on a white background: Before the PR (light mode): After the PR (light mode): I do see what you mean about it being jarring having a light icon in dark mode. What would you think about leaving the more prominent background, but making it responsive to light mode vs dark mode? |
The main (subjective) problem I have with this PR is that the three dot button background is shown immediately after album/face is hovered. When you hover mouse across multiple albums that moving big three dot button is like crosshair and distorts the impression of elegance. I would preffer to not show the three dot button background immediately when album/face is hovered, but when the button itself is hovered, like it was before and also Google Photos does it. The three dot button background color is just minor thing, it can stay as it is now always in light theme. Thanks for consideration my proposal. |
@ben-basten The only change you need to do is to change 2024-05-04.08.52.44.mp4 |
@waclaw66 I have another icon-related PR in the works, and I can add it there once I get some time to test it out. |
Description
Migrate all usages of
IconButton
toCircleIconButton
, or other similar alternatives.In my opinion,
CircleIconButton
is preferred because its colors are consistent with other buttons throughout the application, implementation is simpler, and accessible labeling is built-in by default.New in this PR:
type
prop toCircleIconButton
, to make it more flexible for usage in any scenario. This was a required change to get the search bar buttons working as expected.Expand for screenshots
Light theme toggle
Dark theme toggle
Album options ellipses
Hover over search button
How Has This Been Tested?
Checked the buttons in light mode, light mode hovered, dark mode, and dark mode hovered.
There are also different icons in the top navigation in narrow mobile views, so the same testing needs to be done there too.
Checklist: