-
Notifications
You must be signed in to change notification settings - Fork 1.2k
[TextField, Radio, Checkbox] Add styles for AI generated values #10819
Conversation
0eb5d7b
to
b5af474
Compare
47c497d
to
b553148
Compare
b553148
to
30620d8
Compare
30620d8
to
e450744
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.
Code looks good but there are a lot of hardcoded colors. Can we add these to the system where they make sense? cc @sarahill
I noticed hard coded values in another PR. I'm chatting with @jonrundle today and will bring this up. |
yep! got some details on how we can clean this up, was on my list for us to pair on :D |
d9c56d9
to
5dfd7a4
Compare
0802eef
to
4e9794f
Compare
Updated |
ada46a2
to
b0cf4fe
Compare
I left some comments in the Figma file re: color. The new border color doesn't pass contrast requirements for interactive elements since these do rely on the border to give the element shape. |
b0cf4fe
to
7323d98
Compare
I chatted with Jon earlier today and he mentioned you all on are a time constraint for the beta release so these suggestions are to help us meet that while keeping connected to the system and making it easier for us to continue to iterate on these designs. Re: tokens vs hard coded values - As I mentioned above, I chatted with Jon earlier about the new and updated tokens. I have some questions about the design so I'm hesitant to make too many breaking changes to the tokens right now. I would also prefer not to use hard coded values. I put together a suggestion of how we might approach the tokens here in figma Things to note:
This would keep things connected to the tokens so we can easily update later. |
@sarahill awesome, appreciate it! yeah I'm good with updating surface for now to just use that across both cases. Is that something your team can help get updated or is that something our team should help get updated? |
a248d5d
to
3118dcd
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.
I'm still seeing the weird hover behaviour https://screenshot.click/30-35-q5w1g-wpqrp.mp4
@sarahill What should the border color be on hover?
Should be |
🤔 here's the hover state with could it be possible that it needs to run a build command? just throwing ideas here |
For Storybook:
|
3118dcd
to
f67eb0b
Compare
✅ I assume it's for
✅ |
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 on the design side ✅
/snapit |
🫰✨ Thanks @matallo! Your snapshots have been published to npm. Test the snapshots by updating your yarn add @shopify/polaris-migrator@0.0.0-snapshot-release-20231031181630 yarn add @shopify/polaris@0.0.0-snapshot-release-20231031181630 yarn add @shopify/polaris-tokens@0.0.0-snapshot-release-20231031181630 yarn add @shopify/stylelint-polaris@0.0.0-snapshot-release-20231031181630 |
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.
Changes look good. Just seeing one thing where the active state on radio is black. Not sure if that's a blocker or intentional
radio.mp4
Personally don't think this is a big deal since these would never be clicked by the user—they'd always be auto-highlighted by sidekick |
This PR was opened by the [Changesets release](https://github.com/changesets/action) GitHub action. When you're ready to do a release, you can merge this and the packages will be published to npm automatically. If you're not ready to do a release yet, that's fine, whenever you add more changesets to main, this PR will be updated. # Releases ## @shopify/polaris@12.1.0 ### Minor Changes - [#10819](#10819) [`460c48cfa`](460c48c) Thanks [@matallo](https://github.com/matallo)! - - Added `tone` prop with `magic` value to `TextField` - Added `tone` prop with `magic` value to `ChoiceList` - Added `tone` prop with `magic` value to `Checkbox` ### Patch Changes - [#11079](#11079) [`9c0433c02`](9c0433c) Thanks [@kyledurand](https://github.com/kyledurand)! - Added tests for destructive mapping in `Page` and `Modal` - [#11081](#11081) [`c0be502a8`](c0be502) Thanks [@kyledurand](https://github.com/kyledurand)! - Updated Page titleMetadata documentation ## polaris.shopify.com@0.60.3 ### Patch Changes - [#11083](#11083) [`54dcc984b`](54dcc98) Thanks [@sam-b-rose](https://github.com/sam-b-rose)! - Fixed typos in Design > Icons section created from MDX layout refactor - Updated dependencies \[[`460c48cfa`](460c48c), [`9c0433c02`](9c0433c), [`c0be502a8`](c0be502)]: - @shopify/polaris@12.1.0 Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
<!-- ☝️How to write a good PR title: - Prefix it with [ComponentName] (if applicable), for example: [Button] - Start with a verb, for example: Add, Delete, Improve, Fix… - Give as much context as necessary and as little as possible - Open it as a draft if it’s a work in progress --> ### WHY are these changes introduced? This PR introduces a new tone prop with a possible value "magic" for determining the values in the component have been generated by AI to `Select` component, as well as "magic" and "magic-subdued" values to Text component. It uses the magic color tokens. References Shopify#10152 ### WHAT is this pull request doing? This is a followup of Shopify#10819 and follows the pattern suggested by the Polaris team. ### How to 🎩 In Storybook http://localhost:6006/?path=/story/all-components-select--magic Check with inline label, and active states. 🖥 [Local development instructions](https://github.com/Shopify/polaris/blob/main/README.md#install-dependencies-and-build-workspaces) 🗒 [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 - [x] Tested a [snapshot](https://github.com/Shopify/polaris/blob/main/documentation/Releasing.md#-snapshot-releases) - [x] 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) - [x] 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
<!-- ☝️How to write a good PR title: - Prefix it with [ComponentName] (if applicable), for example: [Button] - Start with a verb, for example: Add, Delete, Improve, Fix… - Give as much context as necessary and as little as possible - Open it as a draft if it’s a work in progress --> ### WHY are these changes introduced? This PR introduces a new tone prop with a possible value "magic" for determining the values in the component have been generated by AI to `Select` component, as well as "magic" and "magic-subdued" values to Text component. It uses the magic color tokens. References #10152 ### WHAT is this pull request doing? This is a followup of #10819 and follows the pattern suggested by the Polaris team. ### How to 🎩 In Storybook http://localhost:6006/?path=/story/all-components-select--magic Check with inline label, and active states. 🖥 [Local development instructions](https://github.com/Shopify/polaris/blob/main/README.md#install-dependencies-and-build-workspaces) 🗒 [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 - [x] Tested a [snapshot](https://github.com/Shopify/polaris/blob/main/documentation/Releasing.md#-snapshot-releases) - [x] 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) - [x] 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
…ify#10819) <!-- ☝️How to write a good PR title: - Prefix it with [ComponentName] (if applicable), for example: [Button] - Start with a verb, for example: Add, Delete, Improve, Fix… - Give as much context as necessary and as little as possible - Prefix it with [WIP] while it’s a work in progress --> ### WHY are these changes introduced? This PR introduces a new `tone` prop with a possible value "magic" for determining the values in the component have been generated by AI. It uses the `--p-color-text-magic` color token and serves as an example for how the rest of styles in this and other components should be implemented. References Shopify#10152 ### WHAT is this pull request doing? This follows the pattern suggested by the Polaris team. After considering applying the styles with a `data-ai-generated` attribute so it could be modified via DOM manipulation I discarded that option.  Radio in web  Checkbox <img width="571" alt="Screenshot 2023-10-26 at 11 03 40 AM" src="https://github.com/Shopify/polaris/assets/36676/0d5e197a-488b-418b-b698-7d4ad14771b4"> hover <img width="570" alt="Screenshot 2023-10-26 at 11 10 51 AM" src="https://github.com/Shopify/polaris/assets/36676/eb37c2df-1545-45b8-a1a4-83f51cd5c1cb"> checked <img width="573" alt="Screenshot 2023-10-26 at 11 03 59 AM" src="https://github.com/Shopify/polaris/assets/36676/20360362-effe-4982-b382-1a1f70670932"> ### How to 🎩 🖥 [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) - Added a new story in Storybook for showing the variant. ### 🎩 checklist - [x] 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) - [ ] N/A 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
This PR was opened by the [Changesets release](https://github.com/changesets/action) GitHub action. When you're ready to do a release, you can merge this and the packages will be published to npm automatically. If you're not ready to do a release yet, that's fine, whenever you add more changesets to main, this PR will be updated. # Releases ## @shopify/polaris@12.1.0 ### Minor Changes - [Shopify#10819](Shopify#10819) [`460c48cfa`](Shopify@0bf9c03) Thanks [@matallo](https://github.com/matallo)! - - Added `tone` prop with `magic` value to `TextField` - Added `tone` prop with `magic` value to `ChoiceList` - Added `tone` prop with `magic` value to `Checkbox` ### Patch Changes - [Shopify#11079](Shopify#11079) [`9c0433c02`](Shopify@5ee40d5) Thanks [@kyledurand](https://github.com/kyledurand)! - Added tests for destructive mapping in `Page` and `Modal` - [Shopify#11081](Shopify#11081) [`c0be502a8`](Shopify@d500f53) Thanks [@kyledurand](https://github.com/kyledurand)! - Updated Page titleMetadata documentation ## polaris.shopify.com@0.60.3 ### Patch Changes - [Shopify#11083](Shopify#11083) [`54dcc984b`](Shopify@2513d17) Thanks [@sam-b-rose](https://github.com/sam-b-rose)! - Fixed typos in Design > Icons section created from MDX layout refactor - Updated dependencies \[[`460c48cfa`](Shopify@0bf9c03), [`9c0433c02`](Shopify@5ee40d5), [`c0be502a8`](Shopify@d500f53)]: - @shopify/polaris@12.1.0 Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
<!-- ☝️How to write a good PR title: - Prefix it with [ComponentName] (if applicable), for example: [Button] - Start with a verb, for example: Add, Delete, Improve, Fix… - Give as much context as necessary and as little as possible - Open it as a draft if it’s a work in progress --> ### WHY are these changes introduced? This PR introduces a new tone prop with a possible value "magic" for determining the values in the component have been generated by AI to `Select` component, as well as "magic" and "magic-subdued" values to Text component. It uses the magic color tokens. References Shopify#10152 ### WHAT is this pull request doing? This is a followup of Shopify#10819 and follows the pattern suggested by the Polaris team. ### How to 🎩 In Storybook http://localhost:6006/?path=/story/all-components-select--magic Check with inline label, and active states. 🖥 [Local development instructions](https://github.com/Shopify/polaris/blob/main/README.md#install-dependencies-and-build-workspaces) 🗒 [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 - [x] Tested a [snapshot](https://github.com/Shopify/polaris/blob/main/documentation/Releasing.md#-snapshot-releases) - [x] 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) - [x] 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
WHY are these changes introduced?
This PR introduces a new
tone
prop with a possible value "magic" for determining the values in the component have been generated by AI. It uses the--p-color-text-magic
color token and serves as an example for how the rest of styles in this and other components should be implemented.References #10152
WHAT is this pull request doing?
This follows the pattern suggested by the Polaris team. After considering applying the styles with a
data-ai-generated
attribute so it could be modified via DOM manipulation I discarded that option.Radio in web
Checkbox

hover

checked

How to 🎩
🖥 Local development instructions
🗒 General tophatting guidelines
📄 Changelog guidelines
🎩 checklist
README.md
with documentation changes