-
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): added hover and active styles #16523
Conversation
All contributors have signed the DCO. |
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. |
✅ Deploy Preview for v11-carbon-react ready!
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 |
1 similar comment
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.
Hover states look good. I see you added the active styling but for some reason the active styling is not applied when clicking down and holding the mouse down. You can see here I toggle the active styling on and it's correct, but when I click and hold to scroll it doesn't change from the hover styling:
CleanShot.2024-06-17.at.11.47.33.mp4
The contained tabs have the same issue. There's something odd going on with the gradient. I'm not sure if it's related to your changes or not, but holding down the right arrow a second time after it's already scrolled all the way to the right makes it go backwards the other way. CleanShot.2024-06-17.at.11.51.01.mp4You might consider moving these changes into #16522 (review) or moving those changes into this PR so we can make sure it's fully correct with all these changes together. |
@soumyaraju any thoughts on my thoughts in my previous comment? |
Hi @tay1orjones i have corrected the active style for the tabs, could you please check it. Thanks |
@soumyaraju active styles look good, the scroll is still not properly working at the rightmost end of the tab list. The final tab is obscured and can't be viewed. 2024-08-06.at.08.31.18-Components.Tabs.-.Default.Storybook-Google.Chrome.mp4 |
@soumyaraju I redeployed the deploy preview with the latest from |
32edd82
to
c3b2425
Compare
@tay1orjones i have added the fix to the above issue, could you please re-check. Thanks |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #16523 +/- ##
=======================================
Coverage 77.08% 77.08%
=======================================
Files 409 409
Lines 14024 14024
Branches 4355 4307 -48
=======================================
Hits 10810 10810
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.
Functionality looks great, thanks!
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.
Hi @aagonzales, thank you for your review! The background color fade you mentioned has been addressed in another PR : #16522., which I added in the PR description. |
@soumyaraju Ok without that fixed its hard to tell if this version is correct. Perhaps we need to wait until that PR is merged so we can see those changes here. I would not want to merge this glitch in as it is now. @tay1orjones thoughts on merge order? |
Hi @aagonzales @tay1orjones i have rebased and verified the background color fade you mentioned, its removed. Could you re-review this pr and approve my changes. Thanks. |
@aagonzales the other PR (#16522) has been merged and both set of changes should be visible here for you to re-review. |
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.
Yup, everything looks good now! Thanks for fixing all of that.
Thanks @aagonzales for the review, @alisonjoseph could you help me merge these changes. TIA |
37907de
* feat(tabs): added hover and active styles * feat(tabs): revert test style * feat(tabs): active tab style order fix * feat(tabs): issue fix :last tab is being partially hidden * feat(tabs): issue fix :last tab is being partially hidden * feat(tabs): added to contributors list * feat(tabs): rebase --------- Co-authored-by: Soumya Raju <[email protected]> Co-authored-by: Alison Joseph <[email protected]>
Part of #15961
It adds the active and hover states to the line and contained scrollable icon buttons and also fixes the Background missing on left scroll button for line tabs.
Rest of the issues mentioned are handled in a different PR #16522
Changelog
New
Testing / Reviewing