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

#513: Added Customizable Icons for Ratings Field #817

Merged
merged 5 commits into from
Oct 3, 2022
Merged

#513: Added Customizable Icons for Ratings Field #817

merged 5 commits into from
Oct 3, 2022

Conversation

mshamsrainey
Copy link
Contributor

@mshamsrainey mshamsrainey commented Sep 16, 2022

#513: Added Customizable Icons for Rating Field

Demonstration here (no audio): https://www.loom.com/share/b4738b9a8a4e45e0bd9c945d9a203388 (Video demo updated to reflect side drawer and icon size updates)

Note: While working on this ticket, I encountered a bug that I created an issue for here #816. I also received some great design feedback that I wish I had time to work on, but don't think I will at the moment (at least during this one day open source challenge!).

@shamsmosowi shamsmosowi changed the base branch from main to develop September 19, 2022 12:52
@shamsmosowi shamsmosowi self-requested a review September 19, 2022 12:52
@shamsmosowi
Copy link
Member

Hi @mshamsrainey, this looks great!
in terms of the use of the preview fab icon button, it makes sense in the context of the action button, but here it would be more informative for the user to preview how the cell interaction is going to look

@mshamsrainey
Copy link
Contributor Author

@shamsmosowi great call, thanks for that feedback! I'll take a look at making that change either tonight or tomorrow and ping you for another review when I'm finished

@mshamsrainey
Copy link
Contributor Author

Hi @shamsmosowi, I just pushed a new commit changing the settings to give a preview of how the ratings will look using the custom icons.

While working on this, I also realized that the existing code would allow people to leave the text field indicating what they wanted their maximum rating to be blank, or set the maximum rating to something super high like 10,000. So, I added in some simple validation to restrict users to a range 1-20 based on the input props already present in the code.

Let me know what you think!

Copy link
Contributor

@notsidney notsidney left a comment

Choose a reason for hiding this comment

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

This looks good!

You should use the current StarBorderIcon as the default emptyIcon, as it’s higher contrast and improves accessibility.

image

@vercel
Copy link

vercel bot commented Sep 28, 2022

@mshamsrainey is attempting to deploy a commit to the Rowy Team on Vercel.

A member of the Team first needs to authorize it.

@mshamsrainey mshamsrainey requested review from notsidney and removed request for shamsmosowi September 28, 2022 17:12
@mshamsrainey
Copy link
Contributor Author

@notsidney and @shamsmosowi just added those changes to improve contrast, let me know what you think! (@shamsmosowi I accidentally un-requested your review and now I'm not able to request it again, oops!)

@notsidney
Copy link
Contributor

@mshamsrainey Thanks! I’m merging this now and it will be part of the next release 🚀

@notsidney notsidney merged commit b346b89 into rowyio:develop Oct 3, 2022
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.

4 participants