-
Notifications
You must be signed in to change notification settings - Fork 1.2k
[Tabs][Uplift]: Match hover and active colour #9619
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
Conversation
|
/snapit |
|
π«°β¨ Thanks @ginabak! Your snapshots have been published to npm. Test the snapshots by updating your yarn add @shopify/polaris-cli@0.0.0-snapshot-release-20230705201526yarn add @shopify/polaris@0.0.0-snapshot-release-20230705201526 |
| background-color: var(--p-color-bg-secondary-experimental); | ||
| color: var(--p-color-text-primary); | ||
|
|
||
| #{$se23} & { |
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.
This is already under the primary #{$se23} and just needed to be cleaned up.
| color: var(--p-color-text-primary); | ||
| z-index: var(--p-z-index-1); | ||
|
|
||
| #{$se23} & { |
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.
| } | ||
|
|
||
| &:not([aria-disabled='true']):hover { | ||
| background-color: var(--p-color-bg-interactive-subdued-active); |
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.
βΉοΈ This is the only change in the PR (the others are cleanup π§Ό π§Ή).
This is behind the se23:
https://github.com/Shopify/polaris/blob/5e2c3c532a9a0ee7aa6fd49439be118364a44c0d/polaris-react/src/components/Tabs/Tabs.scss#L361
and this follows the Tabs from the Uplift Figma for Tabs ποΈ
| } | ||
|
|
||
| &:not([aria-disabled='true']):active { | ||
| background-color: var(--p-color-bg-interactive-subdued-active); |
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.
From the Uplift Figma for Tabs ποΈ, hover and active were the same.
aveline
left a comment
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.
Background color update and cleanup looks good. Thanks @ginabak! π
sethomas
left a comment
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.
code looks good to me, in regards to your questions:
- no clue (sorry)
- I find it jarring too but unsure the intent :(
@chloerice, you're the best! THANK YOUUU for answering my questions!!!! |
## π£ β€οΈ Hey hey reviewers! π₯³ Would you mind looking at the `Questions` portion below? ππ½ THANK YOU IN ADVANCE. ### WHY are these changes introduced? Following the **[Uplift Figma Link for `Tabs`](https://www.figma.com/file/jLLkmo9r28hiqPvf4s90E4/Polaris-Uplift-Components-%5Bgen3%E2%80%93alpha%5D?node-id=84001%3A38964&mode=dev)** ποΈ , the background colour on `hover` and `active` should be ```CSS background: var(--background-bg-secondary, #F3F3F3); ``` Related Shopify#9543 Discovered through https://github.com/Shopify/web/issues/96536 ### WHAT is this pull request doing? (with uplift) | Before | After | |--------|--------| | <img width="566" alt="Screenshot 2023-07-05 at 1 31 29 PM" src="https://github.com/Shopify/polaris/assets/43223543/41da1a18-6f19-4ece-af4d-bba8d5434316"> | <img width="566" alt="Screenshot 2023-07-05 at 1 32 34 PM" src="https://github.com/Shopify/polaris/assets/43223543/96c1239c-d63a-4cb1-a43e-6ddb2ae9cc9d"> | ### β Questions ππ½ 1. I'm noticing that the `More views` font is slightly more bolded - is this what we want? I can't find any thing in the Figma π <img width="250" alt="Screenshot 2023-07-05 at 1 59 53 PM" src="https://github.com/Shopify/polaris/assets/43223543/084499b4-aeff-4280-982b-e3d4780c234d"> 2. At a certain breakpoint, `More views` disappears, the tabs get slightly larger, and then a scroll appears. Is this the desired behaviour? I find it a little jarring. Here's a video: https://github.com/Shopify/polaris/assets/43223543/392582e0-6f59-4449-89b3-cde4a9a42429 ### How to π© π© π [SPIN on web with these changes π - takes you to Orders β€οΈ ](https://admin.web.more-views.gina-bak.us.spin.dev/store/shop1/orders?inContextTimeframe=none) π Ensure that you have a smaller window frame to ensure that the `More views` is triggered π π₯ [Local development instructions](https://github.com/Shopify/polaris/blob/main/README.md#local-development) π [General tophatting guidelines](https://github.com/Shopify/polaris/blob/main/documentation/Tophatting.md) π [Changelog guidelines](https://github.com/Shopify/polaris/blob/main/.github/CONTRIBUTING.md#changelog) ### π© checklist - [ ] Tested on [mobile](https://github.com/Shopify/polaris/blob/main/documentation/Tophatting.md#cross-browser-testing) - [x] Tested on [multiple browsers](https://help.shopify.com/en/manual/shopify-admin/supported-browsers) - [x] Tested for [accessibility](https://github.com/Shopify/polaris/blob/main/documentation/Accessibility%20testing.md) - [ ] Updated the component's `README.md` with documentation changes - [x] [Tophatted documentation](https://github.com/Shopify/polaris/blob/main/documentation/Tophatting%20documentation.md) changes in the style guide
## π£ β€οΈ Hey hey reviewers! π₯³ Would you mind looking at the `Questions` portion below? ππ½ THANK YOU IN ADVANCE. ### WHY are these changes introduced? Following the **[Uplift Figma Link for `Tabs`](https://www.figma.com/file/jLLkmo9r28hiqPvf4s90E4/Polaris-Uplift-Components-%5Bgen3%E2%80%93alpha%5D?node-id=84001%3A38964&mode=dev)** ποΈ , the background colour on `hover` and `active` should be ```CSS background: var(--background-bg-secondary, #F3F3F3); ``` Related Shopify#9543 Discovered through Shopify/web#96536 ### WHAT is this pull request doing? (with uplift) | Before | After | |--------|--------| | <img width="566" alt="Screenshot 2023-07-05 at 1 31 29 PM" src="https://github.com/Shopify/polaris/assets/43223543/41da1a18-6f19-4ece-af4d-bba8d5434316"> | <img width="566" alt="Screenshot 2023-07-05 at 1 32 34 PM" src="https://github.com/Shopify/polaris/assets/43223543/96c1239c-d63a-4cb1-a43e-6ddb2ae9cc9d"> | ### β Questions ππ½ 1. I'm noticing that the `More views` font is slightly more bolded - is this what we want? I can't find any thing in the Figma π <img width="250" alt="Screenshot 2023-07-05 at 1 59 53 PM" src="https://github.com/Shopify/polaris/assets/43223543/084499b4-aeff-4280-982b-e3d4780c234d"> 2. At a certain breakpoint, `More views` disappears, the tabs get slightly larger, and then a scroll appears. Is this the desired behaviour? I find it a little jarring. Here's a video: https://github.com/Shopify/polaris/assets/43223543/392582e0-6f59-4449-89b3-cde4a9a42429 ### How to π© π© π [SPIN on web with these changes π - takes you to Orders β€οΈ ](https://admin.web.more-views.gina-bak.us.spin.dev/store/shop1/orders?inContextTimeframe=none) π Ensure that you have a smaller window frame to ensure that the `More views` is triggered π π₯ [Local development instructions](https://github.com/Shopify/polaris/blob/main/README.md#local-development) π [General tophatting guidelines](https://github.com/Shopify/polaris/blob/main/documentation/Tophatting.md) π [Changelog guidelines](https://github.com/Shopify/polaris/blob/main/.github/CONTRIBUTING.md#changelog) ### π© checklist - [ ] Tested on [mobile](https://github.com/Shopify/polaris/blob/main/documentation/Tophatting.md#cross-browser-testing) - [x] Tested on [multiple browsers](https://help.shopify.com/en/manual/shopify-admin/supported-browsers) - [x] Tested for [accessibility](https://github.com/Shopify/polaris/blob/main/documentation/Accessibility%20testing.md) - [ ] Updated the component's `README.md` with documentation changes - [x] [Tophatted documentation](https://github.com/Shopify/polaris/blob/main/documentation/Tophatting%20documentation.md) changes in the style guide
π£ β€οΈ Hey hey reviewers! π₯³ Would you mind looking at the
Questionsportion below? ππ½ THANK YOU IN ADVANCE.WHY are these changes introduced?
Following the Uplift Figma Link for
TabsποΈ , the background colour onhoverandactiveshould beRelated #9543
Discovered through https://github.com/Shopify/web/issues/96536
WHAT is this pull request doing?
(with uplift)
β Questions ππ½
More viewsfont is slightly more bolded - is this what we want? I can't find any thing in the Figma πMore viewsdisappears, the tabs get slightly larger, and then a scroll appears. Is this the desired behaviour? I find it a little jarring.Here's a video:
More.views.Scroll.mp4
How to π©
π© π SPIN on web with these changes π - takes you to Orders β€οΈ π
Ensure that you have a smaller window frame to ensure that the
More viewsis triggered ππ₯ Local development instructions
π General tophatting guidelines
π Changelog guidelines
π© checklist
README.mdwith documentation changes