-
Notifications
You must be signed in to change notification settings - Fork 100
feat(vote): add vote component #2062
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
base: beta
Are you sure you want to change the base?
Conversation
🦋 Changeset detectedLatest commit: fa90b94 The changes in this PR will be included in the next version bump. Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
✅ Deploy Preview for stacks-svelte ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
✅ Deploy Preview for stacks ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
…#2004) Bumps [stylelint-config-standard](https://github.com/stylelint/stylelint-config-standard) from 38.0.0 to 39.0.1. - [Release notes](https://github.com/stylelint/stylelint-config-standard/releases) - [Changelog](https://github.com/stylelint/stylelint-config-standard/blob/main/CHANGELOG.md) - [Commits](stylelint/stylelint-config-standard@38.0.0...39.0.1) --- updated-dependencies: - dependency-name: stylelint-config-standard dependency-version: 39.0.1 dependency-type: direct:development update-type: version-update:semver-major ... Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
mukunku
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.
|
Downvote icons are available now (the vote icon but direction set to down) |
|
Confirming we don't have a use case currently where we have a horizontal version with both up and downvotes. We only have the horizontal single vote so wondering if we should just not show the version with 2 votes on horizontal (people shouldn't use this). I'm ok keeping the version you have though where both up/down are possible and the focus/hover stays on one side. I think it still works fine even with just the upvote and it would save us time later if product decides to add in downvotes to comments/replies. |
CGuindon
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.
New downvote icon needed (overall looks good though!)
Nice catch @mukunku! I built it with this in mind originally but completely spaced when making the examples and Svelte component. Fixed!
@CGuindon Sal pointed out that I missed the configuration in the Figma. I've updated the horizontal orientation to match what's in Figma where the element is just one upvote button that includes the vote count. If/when we want to allow for downvotes on the horizontal orientation, we'll be able to do that pretty trivially.
The standard (20x20) downvote icon is available but we need the 16px ones for this component. We can still merge without them and once they're published and the icons library dependency is updated, this component will display them with no further changes besides regenerating the test images. |
mukunku
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.
LGTM!
giamir
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.
Great work @dancormier. I really like this component.
The only thing I noticed is that we automatically update the total number but not the upvotes and downvotes numbers.
I am actually unsure if updating the different counts is something we should do in the design system component directly or rather have the consumers take care of it. We don't have control over what the server returns and how the API looks like and we risk to show an incorrect value to the user (imagine a call is done to the server to register an upvote and the server come back with a different score because in the meantime somebody else has also casted a vote, etc...).
That said I am happy to merge it as is and maybe have those conversations with the first team implementing the component.
A small nit. the prop we call total in the component is called score in our APIs (check for example the schema of the response for questions/id). It would be nice probably to stick to score also here but total is ok too. 🙂
|
The Svelte component was built to update the count and wait for a promise to resolve to confirm the vote count was updated on the server-side. It makes sense to do the same thing for the separate counts. |


SPARK-103
Figma
Docs (Stacks Classic)
Storybook (Stacks Svelte)
This PR adds a vote component to Stacks Classic and Stacks Svelte.
Note
At the time of writing, downvote icons are not yet design so are instead shown using the placeholder icons. Once the downvote icons are updated in the stacks-icons packages, the correct icons should show.
Screenshots
Base
Light
Dark
Light HC
Dark HC
Expanded
Horizontal
Voted