Skip to content
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.

Sign up for GitHub

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

Jump to bottom

feat: Add new tokens to SkuSelector [FS-379] #202

Merged
merged 7 commits into from
Aug 16, 2022
Merged

feat: Add new tokens to SkuSelector [FS-379] #202

merged 7 commits into from
Aug 16, 2022

Conversation

lucasfp13
Copy link
Contributor

@lucasfp13 lucasfp13 commented Aug 9, 2022 •
edited
Loading

What's the purpose of this pull request?

This PR intends to apply new tokens to SkuSelector based on the new Theming structure: new scoped tokens linked to global ones.

How does it work?

This PR uses local variables to parameterize the SkuSelector properties, then connects these scoped tokens to the global ones.

How to test it?

These changes should not affect the SkuSelector behavior. It can be tested through Storybook, inside the SkuSelector page.

Checklist

PR Title and Commit Messages

  • PR title and commit messages follow the Conventional Commits specification
    • Available prefixes: feat, fix, chore, docs, style, refactor, perf, and test

PR Description

  • Added a label according to the PR goal - Breaking change, Features, Bug fixes, Chore, Documentation, Style changes, Refactoring, Performance, and Test
  • Added the component, hook, or path name in-between backticks (``) - if applicable, e.g., ComponentName component, useWindowDimensions hook

Documentation

  • PR description
  • Added to/Updated the Storybook - if applicable

@lucasfp13 lucasfp13 added the Features New feature or request label Aug 9, 2022
@lucasfp13 lucasfp13 self-assigned this Aug 9, 2022
@vercel
Copy link

vercel bot commented Aug 9, 2022 •
edited
Loading

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated
nextjs-store-storybook ✅ Ready (Inspect) Visit Preview Aug 16, 2022 at 0:47AM (UTC)

@vercel vercel bot temporarily deployed to Preview August 9, 2022 14:00 Inactive
@vtex-sites
Copy link

vtex-sites bot commented Aug 9, 2022 •
edited
Loading

Preview is ready

This pull request generated a Preview

👀   Preview: https://preview-202--nextjs.preview.vtex.app
🔬   Go deeper by inspecting the Build Logs
📝   based on commit 55449f1
⚠️   User login might not work on this preview.

@vtex-sites
Copy link

vtex-sites bot commented Aug 9, 2022 •
edited
Loading

Lighthouse Reports

Here are the Lighthouse reports of this Pull Request

📝 Based on commit 55449f1

Lighthouse Report by page
📎   /
📎   /apple-magic-mouse/p
📎   /office

@lucasfp13 lucasfp13 force-pushed the feat/FS-379-add-new-tokens-to-sku-selector branch from 5780d3a to 9bd7a5c Compare August 11, 2022 17:02
@vercel vercel bot temporarily deployed to Preview August 11, 2022 17:06 Inactive
@lucasfp13 lucasfp13 marked this pull request as ready for review August 11, 2022 17:09
hellofanny
hellofanny reviewed Aug 12, 2022
View reviewed changes
src/components/ui/SkuSelector/sku-selector.module.scss
@@ -0,0 +1,172 @@
@import "src/styles/scaffold";

@mixin sku-selector-focus-ring {
Copy link
Contributor

Choose a reason for hiding this comment

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

should we move this to utilities.scss file? Or specific enough to leave it here? 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was thinking about this too, move this to another file. But is way specific indeed, idk if it would make sense..

Copy link
Contributor

Choose a reason for hiding this comment

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

oh okay. Let's keep it here then... 👍

hellofanny
hellofanny reviewed Aug 12, 2022
View reviewed changes
src/components/ui/SkuSelector/SkuSelector.stories.mdx

<Canvas>
<Story
name="overview-default-1"
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
name="overview-default-1"
name="overview-default"

@vercel vercel bot temporarily deployed to Preview August 12, 2022 17:23 Inactive
eduardoformiga
eduardoformiga reviewed Aug 12, 2022
View reviewed changes
Copy link
Member

@eduardoformiga eduardoformiga left a comment

Choose a reason for hiding this comment

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

Good Job! 👏

While navigating through the storybook I wonder if we should use the Selectors as the root component, and the SkuSelector listed as a Compound Components as we can see in other components (e.g Accordion, Dropdown). What do you think?
Screen Shot 2022-08-12 at 19 08 26

src/components/ui/SkuSelector/Selectors.stories.mdx Outdated Show resolved Hide resolved
src/components/ui/SkuSelector/Selectors.stories.mdx Outdated Show resolved Hide resolved
src/components/ui/SkuSelector/SkuSelector.tsx Outdated Show resolved Hide resolved
@lucasfp13
Copy link
Contributor Author

lucasfp13 commented Aug 13, 2022 •
edited
Loading

While navigating through the storybook I wonder if we should use the Selectors as the root component, and the SkuSelector listed as a Compound Components as we can see in other components (e.g Accordion, Dropdown). What do you think?

Humm, idk about it because of the self-titled thing, I think SkuSelector with the overview about the Selectors would be at least confusing. We can discuss it with the team 👍🏼

@lucasfp13 lucasfp13 force-pushed the feat/FS-379-add-new-tokens-to-sku-selector branch from f0a06de to e73808d Compare August 13, 2022 01:47
@vercel vercel bot temporarily deployed to Preview August 13, 2022 01:49 Inactive
@hellofanny
Copy link
Contributor

Well Done 🌟

@eduardoformiga
Copy link
Member

While navigating through the storybook I wonder if we should use the Selectors as the root component, and the SkuSelector listed as a Compound Components as we can see in other components (e.g Accordion, Dropdown). What do you think?

Humm, idk about it because of the self-titled thing, I think SkuSelector with the overview about the Selectors would be at least confusing. We can discuss it with the team 👍🏼

I think we can go ahead and think about it afterward!

@lucasfp13 lucasfp13 force-pushed the feat/FS-379-add-new-tokens-to-sku-selector branch from e73808d to 55449f1 Compare August 16, 2022 00:43
@vercel vercel bot temporarily deployed to Preview August 16, 2022 00:47 Inactive
@lucasfp13 lucasfp13 merged commit 78f3221 into main Aug 16, 2022
@lucasfp13 lucasfp13 deleted the feat/FS-379-add-new-tokens-to-sku-selector branch August 16, 2022 00:53
lucasfp13 added a commit to vtex-sites/gatsby.store that referenced this pull request Aug 16, 2022
@lucasfp13
chore: Apply patch from vtex-sites/nextjs.store#202
d5995ec
@lucasfp13 lucasfp13 mentioned this pull request Aug 16, 2022
Merged
7 tasks
lucasfp13 added a commit to vtex-sites/gatsby.store that referenced this pull request Aug 17, 2022
@lucasfp13 @hellofanny
feat: Add new tokens to SkuSelector (#179)
04c08ab 
* chore: Apply patch from vtex-sites/nextjs.store#202

* fix: `SkuSelector` exports

Co-authored-by: Fanny Chien <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@victorhmp victorhmp victorhmp approved these changes

@eduardoformiga eduardoformiga eduardoformiga approved these changes

@hellofanny hellofanny hellofanny approved these changes

Assignees

@lucasfp13 lucasfp13

Labels
Features New feature or request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@lucasfp13 @hellofanny @eduardoformiga @victorhmp