-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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(tabs): take out fade from scroll button #16522
feat(tabs): take out fade from scroll button #16522
Conversation
All contributors have signed the DCO. |
✅ Deploy Preview for v11-carbon-react ready!Built without sensitive environment variables
To edit notification comments on pull requests, go to your Netlify site configuration. |
✅ Deploy Preview for carbon-elements ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
I have read the DCO document and I hereby sign the DCO. |
✅ Deploy Preview for v11-carbon-react ready!Built without sensitive environment variables
To edit notification comments on pull requests, go to your Netlify site configuration. |
✅ Deploy Preview for carbon-elements ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
recheck |
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.
There is a small gap on the left scroll button that allows you to see the text when you shouldn't be able to.
CleanShot.2024-06-17.at.11.41.24.mp4
This might be able to be resolved by updating with the latest from main
, but there are conflicts that need to be resolved.
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.
I'm seeing a non-transparent background on the default tabs, but I know that there are open issues about this:
We should keep the fade on the default tabs and only take the fade out on contained tabs only.
Since the disabled border token is higher contrast, let's update the $border-disabled
to $border-subtle
in the default tabs.
4c84679
to
36c36ed
Compare
Hi @tay1orjones I have rebased and resolved all conflicts |
Hey @kingtraceyj
So could you confirm if I need to change the colour to $border-subtle? |
@divya-281 could you try out this idea for a fix and see if we could fix the transparent left button issue in this PR? #15924 (comment) |
Hi @tay1orjones I added this suggestion also, and it fixed the left arrow button |
@divya-281 looks good, but the border token still needs updated |
Hi @tay1orjones This is how the disabled tabs look like with $border-subtle |
4f32684
to
7e532cf
Compare
@kingtraceyj @tay1orjones I have made the changes, Could you please review. |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #16522 +/- ##
=======================================
Coverage 77.07% 77.07%
=======================================
Files 409 409
Lines 14021 14021
Branches 4307 4307
=======================================
Hits 10807 10807
Misses 3044 3044
Partials 170 170 ☔ View full report in Codecov by Sentry. |
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.
Ok the problems previously mentioned look resolved but I noticed some things. The 1px gap between the default line tabs disappears as the container shrinks down. (Kind of hard to see the gif)
The arrow button seems to be a bit delayed in disappearing when dragging the browser back out. Not sure if others are seeing this too.
Hi @tay1orjones these issues already exist in production and seem to be unrelated to my changes so I think I can work on it on another PR if that's okay |
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.
Yeah those are existing issues not related to this PR. @divya-281 could you open up a new issue outlining those issues?
Hi @tay1orjones @aagonzales I have created an issue for this |
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.
Ok, ignoring those other issues this PR looks good. The fade looks properly removed.
Closes #
It fixes these two issues from the issue #15961
Changelog
Changed
Left scroll button:
Right scroll button:
The disabled token is already apprearing here:
Testing / Reviewing
Start story book with
yarn storybook
Narrow your screen so that the scrollable button is visible