Skip to content

Update EuiCodeBlock's children proptype#2324

Merged
chandlerprall merged 2 commits intoelastic:masterfrom
chandlerprall:bug/2322-euicodeblock-children-types
Sep 10, 2019
Merged

Update EuiCodeBlock's children proptype#2324
chandlerprall merged 2 commits intoelastic:masterfrom
chandlerprall:bug/2322-euicodeblock-children-types

Conversation

@chandlerprall
Copy link
Contributor

@chandlerprall chandlerprall commented Sep 10, 2019

Summary

Fixes #2322

EuiCodeBlock's children proptype has been node when the component internally treated it as string. This PR aligns the proptype to the component's intent while maintaining backwards compatibility for any existing implementations passing malformed children.

The bugs:

  • "copy" button expects the value to be a single string
  • dynamic content does not get reflected

The component needs to operate on a single string, but this constrains the developer experience to working in a non-React way when "interpolating" variables

<EuiCodeBlock>Hello {name}</EuiCodeBlock> creates a children of ['Hello', name], and forcing children to be a single string would require <EuiCodeBlock>{'Hello ' + name}</EuiCodeBlock> or some other way to pre-compute the string.

To support the React Way™️, EuiCodeBlock now takes a string or array of strings which it concatenates together. The block of code doing this maintains backwards compatibility for instances which violate the proptype to avoid making this a breaking change with potentially very difficult broken cases to locate. When this is converted to TypeScript the invalid uses become compile-time errors and we can remove the backwards compatibility.

Checklist

- [ ] Checked in dark mode
- [ ] Checked in mobile
- [ ] Checked in IE11 and Firefox
- [ ] Props have proper autodocs
- [ ] Added documentation examples
- [ ] Added or updated jest tests

  • Checked for breaking changes and labeled appropriately
    - [ ] Checked for accessibility including keyboard-only and screenreader modes
  • A changelog entry exists and is marked appropriately

Copy link
Contributor

@thompsongl thompsongl left a comment

Choose a reason for hiding this comment

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

LGTM

@chandlerprall chandlerprall merged commit cceb6cc into elastic:master Sep 10, 2019
@chandlerprall chandlerprall deleted the bug/2322-euicodeblock-children-types branch September 10, 2019 21:04
@cchaos cchaos mentioned this pull request Sep 11, 2019
5 tasks
@cchaos
Copy link
Contributor

cchaos commented Sep 13, 2019

@chandlerprall I now seem to be getting these errors in the docs related to children of EuiCode

bundle.js:318941 Warning: Failed prop type: Invalid prop `children` supplied to `EuiCode`.
    in EuiCode (created by GuideSection)
    in GuideSection (created by Connect(GuideSection))
    in Connect(GuideSection)
    in GuidePage (created by component)
    in EuiErrorBoundary (created by component)
    in component (created by RouterContext)
    in EuiContext (created by AppView)
    in div (created by AppView)
    in div (created by EuiPageBody)
    in EuiPageBody (created by AppView)
    in div (created by EuiPage)
    in EuiPage (created by AppView)
    in div (created by AppView)
    in AppView (created by Connect(AppView))
    in Connect(AppView) (created by RouterContext)
    in RouterContext (created by Router)
    in Router
    in Provider
printWarning @ bundle.js:318941
bundle.js:318941 Warning: Failed prop type: Invalid prop `children` supplied to `EuiCodeBlockImpl`.
    in EuiCodeBlockImpl (created by EuiCode)
    in EuiCode (created by GuideSection)
    in span (created by EuiTableRowCell)
    in div (created by EuiTableRowCell)
    in td (created by EuiTableRowCell)
    in EuiTableRowCell (created by GuideSection)
    in tr (created by EuiTableRow)
    in EuiTableRow (created by GuideSection)
    in tbody (created by EuiTableBody)
    in EuiTableBody (created by GuideSection)
    in table (created by EuiTable)
    in EuiTable (created by GuideSection)
    in EuiErrorBoundary (created by GuideSection)
    in div (created by GuideSection)
    in GuideSection (created by Connect(GuideSection))
    in Connect(GuideSection)
    in GuidePage (created by component)
    in EuiErrorBoundary (created by component)
    in component (created by RouterContext)
    in EuiContext (created by AppView)
    in div (created by AppView)
    in div (created by EuiPageBody)
    in EuiPageBody (created by AppView)
    in div (created by EuiPage)
    in EuiPage (created by AppView)
    in div (created by AppView)
    in AppView (created by Connect(AppView))
    in Connect(AppView) (created by RouterContext)
    in RouterContext (created by Router)
    in Router
    in Provider
printWarning @ bundle.js:318941

@chandlerprall
Copy link
Contributor Author

@cchaos which docs page were you looking at? Something you were creating a new example for? If so, I think you were passing the wrong value to the example's code param.

@cchaos
Copy link
Contributor

cchaos commented Sep 13, 2019

Hmmm, I was just on the Button component page adding more EuiButtonGroups, but maybe it was just in a weird state. I'll keep an eye out.

chandlerprall added a commit to chandlerprall/eui that referenced this pull request Jan 16, 2020
chandlerprall added a commit that referenced this pull request Feb 3, 2020
* Effective reverting of #2324

* Made EuiCodeBlock updates React-safe

* Updated all EuiCode* tests working as full mounts

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

EuiCodeBlock doesn't re-render when children prop changes.

3 participants