Conversation
|
The other thing that I've run into too, if we want to beef up this PR, is that we don't always do the component extensions like extending |
|
Preview documentation changes for this PR: https://eui.elastic.co/pr_4517/ |
|
@cchaos I'll chat with you next week. I went a little nuts and fixed all the things. Don't worry, I was playing music loud and having fun using vim macros. I'll drink plenty of bourbon tonight to make up for it. |
snide
added
the
breaking change
|
Preview documentation changes for this PR: https://eui.elastic.co/pr_4517/ |
|
jenkins test this |
|
Preview documentation changes for this PR: https://eui.elastic.co/pr_4517/ |
I like this idea. It seems like the typical pattern for internal props is to just omit a prefix, which isn't actually very helpful. |
|
Same! I've also started doing this with specifically internal JS files by starting them with a leading underscore like |
cchaos
left a comment
cchaos
left a comment
There was a problem hiding this comment.
Did a quick scan and LGTM!
thompsongl
left a comment
thompsongl
left a comment
There was a problem hiding this comment.
🙏 Thanks for this!
I think it makes sense to export types for anything in the top-level API, which includes things like EuiHue and EuiSaturation that I thought were internal but aren't.
| export { | ||
| EuiColorPickerSwatch, | ||
| EuiColorPickerSwatchProps, | ||
| } from './color_picker_swatch'; | ||
| export { EuiHue, EuiHueProps } from './hue'; | ||
| export { EuiSaturation, EuiSaturationProps } from './saturation'; |
There was a problem hiding this comment.
These three are really just internal components. I guess there's on harm in exporting the types, though.
cchaos
removed
the
breaking change
Co-authored-by: Caroline Horn <549577+cchaos@users.noreply.github.com>
|
Preview documentation changes for this PR: https://eui.elastic.co/pr_4517/ |
Co-authored-by: Caroline Horn <549577+cchaos@users.noreply.github.com>
|
Preview documentation changes for this PR: https://eui.elastic.co/pr_4517/ |
|
Preview documentation changes for this PR: https://eui.elastic.co/pr_4517/ |
|
Feedback addressed. Merging. |
4 participants
Summary
Killed some time while waiting on my kids to come home from school. This will need discussion.
While building out doc components I ran into a couple places where the props were not properly exported. I decided to do an audit of what EUI exports and fixed the changes. I also corrected some naming inconsistencies to match our normal patterns (ex: using
_EuiSomethingfor internally referenced props).Changes
import { EuiComponentNameProps } from @elastic/eui/without reaching into source.Breaking changes
EuiComponentNamePropsnaming schema for all exported props. In less than a dozen instances, we exported them simply asProps. We weren't really exporting these in a lot of cases, but I think it's worthwhile to label this as a breaking change. My guess is this will cause a minor upgrade pain in Kibana/Cloud.Checklist
Check against all themes for compatibility in both light and dark modeChecked in mobileChecked in Chrome, Safari, Edge, and FirefoxAdded documentationChecked Code Sandbox works for the any docs examplesAdded or updated jest testsChecked for accessibility including keyboard-only and screenreader modes