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

Docs for EuiIcon new abilities #1889

Merged
merged 4 commits into from
May 1, 2019

Conversation

snide
Copy link
Contributor

@snide snide commented Apr 26, 2019

Summary

WIP. Still needs:

  • Apply copy logic to all icon examples
  • Snippets for each section

For a later PR i'd like to

  • Make a guideline that describes good usage and authoring patterns
  • Possibly break out EuiToken into its own page

@cchaos / @chandlerprall might want to check if we're ok with the lack of animation with non EUI icons. It's prolly OK? Also, @cchaos / @ryankeairns do you think we should copy the component or string for each icon? I couldn't decide which was more useful, which is why I've only applied it to one example in here.

image

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

@snide snide changed the title [WIP] Docs for EuiIcon new abilities Docs for EuiIcon new abilities Apr 30, 2019
@snide snide requested review from cchaos and chandlerprall April 30, 2019 20:53
@snide
Copy link
Contributor Author

snide commented Apr 30, 2019

K, docs ready for review. Added various snippets, copy commands and the svg examples. @chandlerprall is looking into EuiIcon being noisy about accepting strings.

@cchaos
Copy link
Contributor

cchaos commented May 1, 2019

do you think we should copy the component or string for each icon?

I think it should be the string name for couple of reasons:

a. The snippet tab should hold the easily copyable component code
b. Making the panel clickable makes the text unselectable and therefore they can never just select and copy the icon string

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.

Mainly a couple word edits. But also looks like we need to update all the iconType prop types in other components to allow the new list.

I also will have a follow-up PR for this branch to allow for multiple snippets since that'd be really useful here.

src-docs/src/views/icon/icon_example.js Outdated Show resolved Hide resolved
src-docs/src/views/icon/icon_example.js Outdated Show resolved Hide resolved
src-docs/src/views/icon/icon_example.js Outdated Show resolved Hide resolved
<EuiButton iconType="https://upload.wikimedia.org/wikipedia/commons/9/9f/Vimlogo.svg">http://some.svg</EuiButton>
</EuiFlexItem>
<EuiFlexItem grow={false}>
<EuiButton iconType={reactSvg}>{`{reactSvg}`}</EuiButton>
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like these are throwing console errors:

Screen Shot 2019-05-01 at 11 07 40 AM

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry, forgot to mention yesterday that @chandlerprall is working on this part.

src-docs/src/views/icon/icons.js Show resolved Hide resolved
src-docs/src/views/icon/ml.js Show resolved Hide resolved
@snide
Copy link
Contributor Author

snide commented May 1, 2019

Feedback addressed. Since this merges to a feature branch, we should be safe merging so @chandlerprall can address the prop typing issues separately.

@snide
Copy link
Contributor Author

snide commented May 1, 2019

Merging. Last to do for the feature branch is fix the console noise and the ie11 issues.

@snide snide merged commit 8b9f0df into elastic:feature/dynamic-euiicon May 1, 2019
@snide snide deleted the docs/euiicon branch May 1, 2019 18:23
chandlerprall added a commit that referenced this pull request May 7, 2019
…rnal urls (#1924)

* Feature/icon breakapart (#1856)

* dynamic import

* Make the icon kinda work

* progress

* generate tsx from svg

* Build and commit icons TSX

* Updated Icon snapshots

* Updated EuiIcon build to again work in dependant projects

* Create a single eui.js build, bundling EuiIcon's dynamic import into the build

* Tests are passing

* Add a loading class to EuiIcon

* Added -isLoaded and using animations for fading

* update snapshots

* Remove background color from isLoaded state

* PR feedback

* Docs for EuiIcon new abilities (#1889)

Add docs for the custom svg abilities in EuiIcon

* DOCS ONLY: Allow multiple snippets (#1908)

* Update IconType and its proptype usage (#1913)

* Expand IconType to include string

* Update EuiIcon IconType to include Element, fix some TS issues, widen the EuiIcon IconType proptype

* Swap Vim example logo out for SVG example logo, which contains a viewBox for IE11 compat

* changelog
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.

2 participants