-
Notifications
You must be signed in to change notification settings - Fork 13
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(sbb-tab-group): tab titles redesign #1975
Conversation
Codecov Report
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. @@ Coverage Diff @@
## master #1975 +/- ##
===========================================
- Coverage 54.85% 36.66% -18.20%
===========================================
Files 49 288 +239
Lines 1659 10905 +9246
Branches 406 2385 +1979
===========================================
+ Hits 910 3998 +3088
- Misses 671 6649 +5978
- Partials 78 258 +180
... and 239 files with indirect coverage changes 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
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 have to go deeper in code on Monday, however some notes:
- focus state in not drawn in Figma (now it's pretty squared)
- when pressed, title text should change position (as in the button)
I also noticed that the content panel can be focused and that a did-change event is triggered as soon as storybook loads the component, but it happens the same on master for both, so I assume it's ok.. :)
b88383d
to
d888550
Compare
Hi @DavideMininni-Fincons, thanks for the feedback.
|
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.
Looks good, just something to check in stories.
d888550
to
e4d8ad3
Compare
@dauriamarco thank you for fixing the issues. everything is fine now. |
e4d8ad3
to
3716807
Compare
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.
Basically LGTM 👍
One issue about the header underline
3716807
to
bd2668c
Compare
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.
LGTM 👍
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.
Looks good, thanks! :)
Closes #1907