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.

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: add rating field #2636

Open
wants to merge 11 commits into
base: main
Choose a base branch
from
Open

feat: add rating field #2636

wants to merge 11 commits into from

Conversation

artursantiago
Copy link
Collaborator

@artursantiago artursantiago commented Jan 28, 2025

⚠️ THIS PR DEPENDS ON PR#2635 ⚠️

What's the purpose of this pull request?

This pull request adds the RatingField component, which will initially be used in the modal for adding reviews to a product.

How it works?

This PR creates a new molecule component called RatingField. In addition to the "input" fields for the actionable rating, it can also receive an id, label, and error via props to be displayed in the form. The component uses the Rating component with the actionable view, passing the onChange handler to it.

How to test it?

Starters Deploy Preview

Preview

References

JIRA TASK: SFS-2078
JIRA TASK: SFS-2077

Figma

image

image

@artursantiago artursantiago added enhancement New feature or request depends on Depends on another PR - Please mention the PR labels Jan 28, 2025
Copy link

codesandbox-ci bot commented Jan 28, 2025

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Copy link

vercel bot commented Jan 28, 2025

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

Name Status Preview Updated (UTC)
faststore-site ✅ Ready (Inspect) Visit Preview Jan 28, 2025 0:47am

@artursantiago artursantiago self-assigned this Jan 28, 2025
@artursantiago artursantiago marked this pull request as ready for review January 30, 2025 17:43
@artursantiago artursantiago requested a review from a team as a code owner January 30, 2025 17:43
@artursantiago artursantiago requested review from renatamottam and renatomaurovtex and removed request for a team January 30, 2025 17:43
@artursantiago
Copy link
Collaborator Author

@hellofanny

I just fixed the preview link. You can test it now.

Copy link
Contributor

@hellofanny hellofanny left a comment

Choose a reason for hiding this comment

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

Nice work 🚀 ! We just need to tweak a few more details. 👍

@@ -22,6 +22,7 @@
// --------------------------------------------------------
// Structural Styles
// --------------------------------------------------------

Copy link
Contributor

Choose a reason for hiding this comment

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

todo: fix the button size, it's currently 48px, and should be 32px.

image

I will suggest adding a new token:

// Button
--fs-rating-button-min-height: var(--fs-spacing-5);

and then on line 34, force the size:

[data-fs-rating-button] {
 --fs-button-small-min-height: var(--fs-rating-button-min-height);

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Alright, I've implemented the suggested change, I had previously kept the token value because I thought it was something set for accessibility, so as not to make the touch area so small

flex-direction: column;

[data-fs-rating-field-label] {
font-size: var(--fs-rating-field-label-size);
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
font-size: var(--fs-rating-field-label-size);
margin-bottom: var(--fs-spacing-0);
font-size: var(--fs-rating-field-label-size);

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done ✅

}

[data-fs-rating-field-error-message] {
font-size: var(--fs-rating-field-error-message-size);
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
font-size: var(--fs-rating-field-error-message-size);
margin-top: var(--fs-spacing-0);
font-size: var(--fs-rating-field-error-message-size);

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done ✅

// --------------------------------------------------------
// Design Tokens for Rating Field
// --------------------------------------------------------

Copy link
Contributor

Choose a reason for hiding this comment

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

After fixing the button size as I mention, we need to adds a little margin in the label and error message. I've left as suggestions.

todo: Can you please update this file indentation just to follow the previous pattern? Thank you!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done ✅

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
depends on Depends on another PR - Please mention the PR enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants