-
Notifications
You must be signed in to change notification settings - Fork 542
Refactor on custom rating icons #903
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
Refactor on custom rating icons #903
Conversation
Also updated formatting of custom rating icons to match default star sz
|
@mshamsrainey is attempting to deploy a commit to the Rowy Team on Vercel. A member of the Team first needs to authorize it. |
notsidney
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.
The issue is that MUI uses different text colors with opacity, hsla(..., opacity) to make it appear not filled in. But WebKit browsers like Safari don’t honor the opacity in text color, and always display emojis with full opacity. (React Native had a similar issue a while back: facebook/react-native#2241)
You can change the global theme component style instead to use opacity and use the same text color as filled icons: https://github.com/rowyio/rowy/blob/enhancement/513-ratings-customized-icons/src/theme/components.tsx#L1376
MuiRating: {
styleOverrides: {
icon: {
// https://github.com/mui/material-ui/issues/32557
"& .MuiSvgIcon-root": { pointerEvents: "auto" },
color: theme.palette.text.secondary,
},
iconEmpty: { opacity: 0.38 },
},
},Then you also don’t need to make Icon into a separate component and keep the code simple like previously.
…:mshamsrainey/rowy into enhancement/513-ratings-customized-icons
|
@notsidney good catch! I hadn't thought to check the issues on React Native. Let me know how this new version looks to you (it's also got some package.json changes that I believe are just a result of merging main into my branch, shouldn't be anything too exciting) |
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.
Thanks @mshamsrainey.
Please don’t merge from main, as it’s a stale branch. Since the PR is into develop, please merge from develop instead. It appears that the package.json and yarn.lock updates downgrade and remove some dependencies, which have prevented the app from running on my machine.
It’s easier to make a new branch off develop when starting a new PR to prevent issues like merging from one of our branches, to prevent issues with dependencies.
I also want to make the point that you’re not expected to look at React Native issues since we don’t use React Native at all. I linked to that issue as an example, after Googling the primary issue of text color.
To mitigate issues with "empty" and standard custom icons looking the same in browsers running on WebKit, I created an Icon component using the logic written in previous PRs that I could then modify to show "empty" custom icons with an explicitly-declared opacity value. No changes to the UI on Chrome, Edge, or Firefox--the only intended change would be for WebKit browsers.