Skip to content
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

Tab UI Improvements #20783

Merged
merged 16 commits into from
Feb 3, 2023
Merged

Tab UI Improvements #20783

merged 16 commits into from
Feb 3, 2023

Conversation

valentinpalkovic
Copy link
Contributor

@valentinpalkovic valentinpalkovic commented Jan 25, 2023

Resolves #20829
Resolves #19989

See: https://www.notion.so/chromatic-ui/Storybook-7-0-UI-update-421d43aa8f9248b29c9dbfa3fdb25dd3#4e86aa0af1af46bab881bae4828a8fd7

What I did

I have improved the Addons Tabs by removing the horizontal scrolling. I have introduced an "Addons" Tooltip instead.

Constraints:

I also wanted to take care of smooth tab navigation for accessibility reasons. And although react-popper-tooltip supports focus triggers, it does not trigger the tooltip on tab focus. Additionally, it is not that easy to continue tabbing into the tooltip. To make that work, we would have to change the tooltip's DOM order a bit and add some tabIndex properties to the items. I tried a lot but failed to integrate tabbing across the tooltip boundary properly. Will invest some time at a later point to properly handle tabbing.

Tabbing into the Tooltip is also blocked by: mohsinulhaq/react-popper-tooltip#147

How to test

Checklist

  • Make sure your changes are tested (stories and/or unit, integration, or end-to-end tests)
  • Make sure to add/update documentation regarding your changes
  • If you are deprecating/removing a feature, make sure to update
    MIGRATION.MD

Maintainers

  • If this PR should be tested against many or all sandboxes,
    make sure to add the ci:merged or ci:daily GH label to it.
  • Make sure this PR contains one of the labels below.

["cleanup", "BREAKING CHANGE", "feature request", "bug", "documentation", "maintenance", "dependencies", "other"]

@valentinpalkovic valentinpalkovic self-assigned this Jan 25, 2023
@valentinpalkovic valentinpalkovic force-pushed the valentin/ui-improvements branch 3 times, most recently from 3a52d23 to 29f6344 Compare January 26, 2023 08:53
@valentinpalkovic valentinpalkovic marked this pull request as ready for review January 26, 2023 09:38
@valentinpalkovic valentinpalkovic marked this pull request as draft January 26, 2023 09:39
@valentinpalkovic valentinpalkovic force-pushed the valentin/ui-improvements branch from 4d3aae7 to 0c92f3a Compare January 26, 2023 11:35
@valentinpalkovic valentinpalkovic added the ci:daily Run the CI jobs that normally run in the daily job. label Jan 26, 2023
@valentinpalkovic valentinpalkovic marked this pull request as ready for review January 26, 2023 14:31
@valentinpalkovic valentinpalkovic added ci:pr and removed ci:daily Run the CI jobs that normally run in the daily job. labels Jan 26, 2023
@socket-security
Copy link

socket-security bot commented Jan 26, 2023

Socket Security Pull Request Report

👍 No new dependency issues detected in pull request

Pull request report summary
Issue Status
Install scripts ✅ 0 issues
Native code ✅ 0 issues
Bin script confusion ✅ 0 issues
Bin script shell injection ✅ 0 issues
Shell access ✅ 0 issues
Uses eval ✅ 0 issues
Unresolved require ✅ 0 issues
Invalid package.json ✅ 0 issues
HTTP dependency ✅ 0 issues
Git dependency ✅ 0 issues
GitHub dependency ✅ 0 issues
New author ✅ 0 issues
Potential typo squat ✅ 0 issues
Known Malware ✅ 0 issues
Telemetry ✅ 0 issues
Protestware/Troll package ✅ 0 issues
AI detected malware ✅ 0 issues
Bot Commands

To ignore an alert, reply with a comment starting with @SocketSecurity ignore followed by a space separated list of package-name@version specifiers. e.g. @SocketSecurity ignore [email protected] [email protected]

Powered by socket.dev

@valentinpalkovic valentinpalkovic force-pushed the valentin/ui-improvements branch 2 times, most recently from ed371b0 to 81e410d Compare January 27, 2023 09:19
@yannbf
Copy link
Member

yannbf commented Jan 27, 2023

This is awesome 👌

I got a few questions, probably design related, so @MichaelArestad your input would be lovely:

  1. Is it intended that the buttons to change orientation of the panel or close it are gone in that mode?
    before:

image

after:
image

  1. just an observation: I think it's a little confusing at first to know which addon I have selected because the title is now "Addons" and I have to hover to see what is selected, but I think it's just something to get used to. Probably biggest source of confusion is the fact that the empty state of addon actions is just a white screen, so you don't know what is actually selected:

image

@yannbf
Copy link
Member

yannbf commented Jan 27, 2023

I detected an issue regarding keyboard navigation, best described in a video format:
https://user-images.githubusercontent.com/1671563/215084077-518603f0-f0a6-4ef4-b4a3-8a727a5108be.mov

I hope this resource is helpful!

@valentinpalkovic
Copy link
Contributor Author

This is awesome 👌

I got a few questions, probably design related, so @MichaelArestad your input would be lovely:

  1. Is it intended that the buttons to change orientation of the panel or close it are gone in that mode?
    before:
image

after: image

  1. just an observation: I think it's a little confusing at first to know which addon I have selected because the title is now "Addons" and I have to hover to see what is selected, but I think it's just something to get used to. Probably biggest source of confusion is the fact that the empty state of addon actions is just a white screen, so you don't know what is actually selected:
image

The buttons should only be gone on mobile phones (<599px). That was also the case before. Or should they stay now always visible?

@valentinpalkovic
Copy link
Contributor Author

I detected an issue regarding keyboard navigation, best described in a video format: https://user-images.githubusercontent.com/1671563/215084077-518603f0-f0a6-4ef4-b4a3-8a727a5108be.mov

I hope this resource is helpful!

Have you seen my PR description? I will revisit that issue later

@yannbf
Copy link
Member

yannbf commented Jan 27, 2023

The buttons should only be gone on mobile phones (<599px). That was also the case before. Or should they stay now always visible?

My bad, I was checking an older build which had the wrong behavior! I was checking desktop with the addon panel positioned on the right.

The latest build does work, though it pushes the right buttons a little further because of a margin-left:
2023-01-27 16 20 52

Have you seen my PR description? I will revisit that issue later

Alright, sounds good! Just keep in mind that apart from the issue of navigating through the tooltip, there's the other where if you tab until the orientation change button, the layout breaks

@yannbf yannbf closed this Jan 27, 2023
@yannbf yannbf reopened this Jan 27, 2023
@valentinpalkovic valentinpalkovic force-pushed the valentin/ui-improvements branch 3 times, most recently from 943bd6d to 98dc019 Compare January 30, 2023 13:43
@MichaelArestad
Copy link
Contributor

  1. Is it intended that the buttons to change orientation of the panel or close it are gone in that mode?

@yannbf Ah! The orientation and close button should always be visible on desktop views. Eventually, when we update our mobile designs, they will also always be visible, but that is not necessary with the current design.

I think it's a little confusing at first to know which addon I have selected because the title is now "Addons"

I agree. Perhaps other options like showing a proper heading could be explored. I have an idea for this, but it might impact the designs of the content of the addon panels. We definitely need a better empty state for Actions.

@yannbf
Copy link
Member

yannbf commented Jan 30, 2023

The changes look pretty good, I am just curious about the breaking changes in @storybook/components and the potential effects of that. I'm not the right person to talk about them unfortunately, but I'm sure others could tell whether the changes are fine or not

@valentinpalkovic valentinpalkovic force-pushed the valentin/ui-improvements branch from 875f9ca to 66a0111 Compare January 31, 2023 11:59
@valentinpalkovic valentinpalkovic force-pushed the valentin/ui-improvements branch from 66a0111 to 2ef671e Compare February 1, 2023 14:16
Copy link
Member

@yannbf yannbf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome work @valentinpalkovic !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Tabs UX [Bug]: addon-interactions + disabled actions breaks storybook
4 participants