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

fix(rating)!: display-value => show-chip, also works now #1481

Merged
merged 3 commits into from
Jan 28, 2021

Conversation

paulcpederson
Copy link
Member

Summary

We need the ability to hide the value of the count/average numbers. I went to add a prop for this and realized it was already there, we just weren't using it 🤦🏻 .

This updates the ratings component to pay attention to the existing display-value prop and adds tests, docs, jsdoc for it as well.

@paulcpederson paulcpederson requested a review from a team as a code owner January 27, 2021 20:55
Copy link
Contributor

@caripizza caripizza left a comment

Choose a reason for hiding this comment

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

Nice! Only question I have is about the displayValue prop name - this new behavior makes me instead think of a name like displayStats (or showChip like Adam had mentioned in Teams), since it applies to either count or average props on the component and not the value prop. 🤔

@paulcpederson
Copy link
Member Author

Yeah, I was going to call it show-summary, but then found this prop, we could definitely change the name as it was completely unused and undocumented 😂

@caripizza
Copy link
Contributor

Yeah, I was going to call it show-summary, but then found this prop, we could definitely change the name as it was completely unused and undocumented 😂

Perfect, can you rename to show-chip? 🐿️

@paulcpederson paulcpederson changed the title fix(rating): use ignored display-value prop fix(rating): display-value now show-chip, also works now Jan 28, 2021
@paulcpederson paulcpederson changed the title fix(rating): display-value now show-chip, also works now fix(rating): display-value => show-chip, also works now Jan 28, 2021
@paulcpederson
Copy link
Member Author

@caripizza updated

@paulcpederson paulcpederson changed the title fix(rating): display-value => show-chip, also works now fix(rating)!: display-value => show-chip, also works now Jan 28, 2021
Copy link
Contributor

@caripizza caripizza left a comment

Choose a reason for hiding this comment

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

lgtm 👍

@paulcpederson paulcpederson merged commit 7bfc32a into master Jan 28, 2021
@paulcpederson paulcpederson deleted the paulcpederson/ratings branch January 28, 2021 20:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants