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

Allow any <color> value in EuiIcon #1370

Merged
merged 4 commits into from
Dec 13, 2018

Conversation

chandlerprall
Copy link
Contributor

@chandlerprall chandlerprall commented Dec 13, 2018

Summary

Allows any color value to be passed to EuiIcon

Checklist

- [ ] This was checked in mobile
- [ ] This was checked in IE11
- [ ] This was checked in dark mode
- [ ] Any props added have proper autodocs

  • Documentation examples were added
  • A changelog entry exists and is marked appropriately
  • This was checked for breaking changes and labeled appropriately
  • Jest tests were updated or added to match the most common scenarios
    - [ ] This was checked against keyboard-only and screenreader scenarios
    - [ ] This required updates to Framer X components

@chandlerprall chandlerprall requested a review from cchaos December 13, 2018 16:43
@@ -1,14 +1,15 @@
## [`master`](https://github.com/elastic/eui/tree/master)

- Reinstate ([#1353](https://github.com/elastic/eui/pull/1353)) `onBlur` action on `EuiComboBox` ([#1364](https://github.com/elastic/eui/pull/1364))
- Convert roughly half of the services to TypeScript ([#1400](https://github.com/elastic/eui/pull/1400))
- Convert roughly half of the services to TypeScript ([#1360](https://github.com/elastic/eui/pull/1360))
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This pointed at the wrong place

Copy link
Contributor

@cchaos cchaos left a comment

Choose a reason for hiding this comment

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

Thanks for jumping on this. I just had a tweak to our docs paragraph.

Here's an example where I used even more values in the docs (but not committing to repo).

screen shot 2018-12-13 at 12 03 16 pm

The last 2 are invalid values but they don't throw an error, they just fallback to text color.

src-docs/src/views/icon/icon_example.js Outdated Show resolved Hide resolved
Copy link
Contributor

@snide snide left a comment

Choose a reason for hiding this comment

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

Prolly need a docs example.

can accept named colors from our palette, a three or six color hex code,
and rgb(r, g, b) or rgba(r, g, b, a) formats. The default behavior is to
inherit the text color as the SVG color <EuiCode>fill</EuiCode> property
via <EuiCode>currentColor</EuiCode> in CSS.
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we add an example of the rgba/hsla to the docs icon_colors.js

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, the autodoc could use a note now. Right now it just says string or enum.

Copy link
Contributor

Choose a reason for hiding this comment

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

@chandlerprall You can pull from one of my examples in the screenshot I pasted.

Copy link
Contributor

Choose a reason for hiding this comment

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

Although, I pasted in a link in my docs suggestion re-write that points to all the valid color types. Maybe we should just reference that and not put more examples of things we don't want people to do in our docs?

Copy link
Contributor

Choose a reason for hiding this comment

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

It also has way more examples of what is/isn't a valid color type.

@@ -649,16 +645,6 @@ export const EuiIcon: SFC<Props> = ({
);
};

function checkValidColor(color: string, type: string) {
Copy link
Contributor

@snide snide Dec 13, 2018

Choose a reason for hiding this comment

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

With this gone it will just take any string now right? Basicly a silent fail if it's malformed? Is everyone OK with that?

Copy link
Contributor

@cchaos cchaos Dec 13, 2018

Choose a reason for hiding this comment

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

Yep! It will just fail on the CSS side as it would if you just normally passed it via style="fill: [color]"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Until the recent TSXification of the component this was a proptype warning, which apparently was ignored by TSVB previously anyway :)

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.

3 participants