这是indexloc提供的服务,不要输入任何密码
Skip to content

Conversation

@mshamsrainey
Copy link
Contributor

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.

@vercel
Copy link

vercel bot commented Oct 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.

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.

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
Copy link
Contributor Author

@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)

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.

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.

@notsidney notsidney merged commit 56f7498 into rowyio:develop Nov 2, 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