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

Refactor svgFill to pass a custom color #576

Closed
chrisvanmook opened this issue May 13, 2020 · 8 comments · Fixed by #594
Closed

Refactor svgFill to pass a custom color #576

chrisvanmook opened this issue May 13, 2020 · 8 comments · Fixed by #594
Assignees
Labels
enhancement New feature or request good first issue Good for newcomers help wanted Extra attention is needed maintenance This issue/PR is related to regular maintainance and does not introduce anything new.
Milestone

Comments

@chrisvanmook
Copy link
Contributor

chrisvanmook commented May 13, 2020

The svgFill method should be refactored to accept a color code only. Also note that the color prop for the Icon component will have to start using this method.

See discussion in #574

@jonkoops jonkoops added enhancement New feature or request good first issue Good for newcomers help wanted Extra attention is needed maintenance This issue/PR is related to regular maintainance and does not introduce anything new. labels May 13, 2020
@jonkoops jonkoops added this to the 1.0.0 milestone May 13, 2020
@jasperswart jasperswart self-assigned this May 13, 2020
@jasperswart
Copy link
Contributor

I will pick this up

@robinpiets
Copy link
Contributor

Maybe we can also change this in the Paragraph component? It has a similar issue.

@jonkoops
Copy link
Contributor

@robinpiets Could you give an example?

@robinpiets
Copy link
Contributor

Now Paragraph uses TypographyStyle, and color is defined like this:

${({ color }) =>
    color &&
    css`
      color: ${themeColor(color)};
    `}

We can change this to pass a string, so we can use it like this:
<Paragraph color={themeColor("tint", "level3")}>...
or
<Paragraph color="#bbb">...

@jonkoops
Copy link
Contributor

Makes sense, I'd like to see that as well. Although I wonder why this prop exists in the first place since it's just a text element. Should we remove it instead?

@robinpiets
Copy link
Contributor

Yeah, it's a strange prop. Shall I make a new PR to change it into this:

${({ color }) =>
  color &&
  css`
      color: ${color};
  `}

@jonkoops
Copy link
Contributor

Sounds good, but let's deprecate the property at the same time. People should just style it in the color they need using styled(Paragraph).

@robinpiets
Copy link
Contributor

I will fix the Paragraph color prop in a separate PR

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request good first issue Good for newcomers help wanted Extra attention is needed maintenance This issue/PR is related to regular maintainance and does not introduce anything new.
4 participants