-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
[UI Framework] Add several kuiLocalNav-related components #11725
[UI Framework] Add several kuiLocalNav-related components #11725
Conversation
className: PropTypes.string, | ||
children: PropTypes.node, | ||
isSelected: PropTypes.bool, | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
missing isDisabled here and below
Can you update the ui_framework/doc_site/src/views/local_nav to use the new react components? |
Holy carp, this will go a long way towards pushing #9758 forward as well! I'll hold off on review until you've updated the doc site with these components per @stacey-gammon's comment. |
@cjcenizal I totally forgot about that PR - not sure how we would reconcile the two. As much as I'd love to see the page header be replaced with react on other pages than the context view, it would be quite the effort. There are usually many angular components embedded in the menu and dropdown which would have to be rewritten to react too. Unless we want to allow "angular directives in react components in angular directives"... 🤔 |
@weltenwort Good point. I doubt I'll have bandwidth to return to that PR for a couple months. By then you'll have used these components in Context View. Maybe that will expose some patterns I can apply to the Angular code, too. |
BTW, we have an unspoken convention of prefixing the titles of UI Framework PRs with |
@weltenwort When you add examples, could you rebase master and then go into }, {
name: 'LocalNav',
component: LocalNavExample,
hasReact: true,
}, { I just added this feature. By adding |
426a0bc
to
d882c1d
Compare
d882c1d
to
6f28f1a
Compare
I converted all examples for which any components exist to React, even if not all elements have corresponding components yet. This means the examples can be easily updated when the remaining components are converted. @stacey-gammon @cjcenizal it should be worth another look, I hope |
KuiLocalNavRowSection, | ||
} from '../../../../components'; | ||
|
||
export default () => ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we make all of these named exports?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
right, that got carried over from the prior art 😉
Failed due to #11499 jenkins, test this |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Aside from switching to named exports, LGTM!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! 🌻
Backports PR elastic#11725 This adds several `LocalNav`-related React components and converts their usage in the docs to React: * `KuiLocalNav` * `KuiLocalNavRow` * `KuiLocalNavRowSection` * `KuiLocalNavTab` * `KuiLocalNavTabs` * `KuiLocalNavTitle`
Backports PR #11725 This adds several `LocalNav`-related React components and converts their usage in the docs to React: * `KuiLocalNav` * `KuiLocalNavRow` * `KuiLocalNavRowSection` * `KuiLocalNavTab` * `KuiLocalNavTabs` * `KuiLocalNavTitle`
This adds several
LocalNav
-related React components I need for the context view:KuiLocalNav
KuiLocalNavRow
KuiLocalNavRowSection
KuiLocalNavTab
KuiLocalNavTabs
KuiLocalNavTitle
Could be seen as a step towards #10312.