Skip to content
This repository was archived by the owner on Sep 30, 2025. It is now read-only.

Conversation

matallo
Copy link
Contributor

@matallo matallo commented Jan 18, 2024

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-bg-fill-magic token.

Screenshot 2024-01-18 at 8 06 59 PM

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-rangeslider--magic

🖥 Local development instructions
🗒 General tophatting guidelines
📄 Changelog guidelines

🎩 checklist

@matallo matallo requested review from a team and mathildebuenerd January 18, 2024 19:15
@matallo matallo force-pushed the matallo/sidekick-rangeslider branch from 1b4eb1a to c58c1ec Compare January 18, 2024 20:26
@alex-page
Copy link
Member

@matallo what is stopping you from adding RangeSlider to magic-ui and doing a CSS override? Does this need to be in Polaris ASAP? Are 3P app developers going to need these features?

Copy link
Contributor

@mathildebuenerd mathildebuenerd left a 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 I wasn't able to tophat. I checked out your branch on my own spin instance but storybook is failing to run. Can you provide a spin link for me to tophat?

@mathildebuenerd
Copy link
Contributor

@matallo what is stopping you from adding RangeSlider to magic-ui and doing a CSS override? Does this need to be in Polaris ASAP? Are 3P app developers going to need these features?

@alex-page, the same prop got added to several Polaris components already, including TextField, Checkbox, RadioButton, Select and ChoiceList.

At the moment I don't think 3P app developers are going to need this feature. In general, do we consider it a bad practice to add a prop to Polaris for something that's required in the admin, but not for 3P app developers?

Copy link
Member

@chloerice chloerice left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✨ 🚀

@matallo
Copy link
Contributor Author

matallo commented Jan 22, 2024

what is stopping you from adding RangeSlider to magic-ui and doing a CSS override?

That can be a solution, we're doing something similar for RichTextEditor. As @mathildebuenerd mentioned I'm following the same pattern as prior components with tone="magic" but there's a wider discussion to be had.

Does this need to be in Polaris ASAP? Are 3P app developers going to need these features?

I don't think so immediately, but I'll let the team using this to answer. Currently the feature is behind a beta flag for the Admin if I'm not wrong.

@aaronccasanova aaronccasanova force-pushed the matallo/sidekick-rangeslider branch from c58c1ec to 8027726 Compare April 19, 2024 21:52
@github-actions github-actions bot added the cla-needed Added by a bot. Contributor needs to sign the CLA Agreement. label Apr 19, 2024
@translation-platform
Copy link
Contributor

Localization quality issues found

The following issues may affect the quality of localized translations if they are not addressed:

  • The value Clear for key Polaris.ActionList.SearchField.clearButtonLabel is very short. Short strings are more likely to be misunderstood by translators without context. Please provide additional context for the translators if possible.
  • The value Search for key Polaris.ActionList.SearchField.search is very short. Short strings are more likely to be misunderstood by translators without context. Please provide additional context for the translators if possible.
  • The value Search actions for key Polaris.ActionList.SearchField.placeholder is very short. Short strings are more likely to be misunderstood by translators without context. Please provide additional context for the translators if possible.

Please look out for other instances of this issue in your PR and fix them as well if possible.

Questions about these messages? Hop in the #help-localization Slack channel.

Copy link
Contributor

Hi! We noticed there hasn’t been activity on this PR in a while. After 30 days, it will close automatically.

If it’s still relevant, or you have updates, comment and let us know. And don’t worry, you can always re-open later if needed.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
cla-needed Added by a bot. Contributor needs to sign the CLA Agreement. no-pr-activity
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants