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

Add support for node name prop to EuiTabbedContent, to bring it up to par with EuiTab #3100

Merged

Conversation

cjcenizal
Copy link
Contributor

EuiTab already supports this, so now users of EuiTabbedContent will also be able to provide a node for the name prop.

image

Checklist

  • Check against all themes for compatibility in both light and dark modes
  • 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

@cjcenizal cjcenizal requested a review from chandlerprall March 17, 2020 04:57
@cjcenizal cjcenizal force-pushed the chore/tabbed-content-node-children branch from 3916c2d to 40aa816 Compare March 17, 2020 04:57
@snide
Copy link
Contributor

snide commented Mar 17, 2020

@cjcenizal This PR is against a pretty old version of EUI. Looks like v14 or so. Looks good otherwise though, I see the current version still has a string there still.

… par with EuiTab.

- Update documentation to illustrate this capability.
@cjcenizal cjcenizal force-pushed the chore/tabbed-content-node-children branch from 40aa816 to 8776864 Compare March 17, 2020 05:36
@cjcenizal
Copy link
Contributor Author

Thanks @snide, looks like I missed a git pull!

@kibanamachine
Copy link

Preview documentation changes for this PR: https://eui.elastic.co/pr_3100/

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.

Thanks, @cjcenizal!

I came across this discrepancy last week and forgot to make an issue for it 👍

CHANGELOG.md Outdated
@@ -15,6 +15,7 @@
- Added `useResizeObserver` hook ([#2991](https://github.com/elastic/eui/pull/2991))
- Added `showColumnSelector.allowHide` and `showColumnSelector.allowReorder` props to `EuiDataGrid` UI configuration ([#2993](https://github.com/elastic/eui/pull/2993))
- Added `EuiMark` component ([#3060](https://github.com/elastic/eui/pull/3060))
- Add support for node `name` prop in `EuiTabbedContent`, to bring it on par with `EuiTab` ([#3100](https://github.com/elastic/eui/pull/3100))
Copy link
Contributor

Choose a reason for hiding this comment

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

I was very confused by the title of this PR, can you change the CL to

Suggested change
- Add support for node `name` prop in `EuiTabbedContent`, to bring it on par with `EuiTab` ([#3100](https://github.com/elastic/eui/pull/3100))
- Changed `name` prop to `ReactNode` type in `EuiTabbedContent` ([#3100](https://github.com/elastic/eui/pull/3100))

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.

Small CL suggestion. LGTM

CHANGELOG.md Outdated Show resolved Hide resolved
Co-Authored-By: Dave Snider <dave.snider@gmail.com>
@cjcenizal
Copy link
Contributor Author

@cchaos I accepted @snide's suggestion which seemed to encompass yours but add a bit more detail. Can you take another look please?

@kibanamachine
Copy link

Preview documentation changes for this PR: https://eui.elastic.co/pr_3100/

@cjcenizal cjcenizal merged commit 217f11b into elastic:master Mar 17, 2020
@cjcenizal cjcenizal deleted the chore/tabbed-content-node-children branch March 17, 2020 20:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants