-
Notifications
You must be signed in to change notification settings - Fork 8.2k
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] Reactify kuiTabs and related CSS components #12302
[UI Framework] Reactify kuiTabs and related CSS components #12302
Conversation
Can one of the admins verify this patch? |
This commit is not intended to be the final one because |
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.
Thanks for doing this @PopradiArpad! I think we can significantly simplify this component. I left some suggestions to try to explain my thoughts. Please let me know if you have any questions!
ui_framework/components/tabs/tab.js
Outdated
return ( | ||
<button | ||
className={classes} | ||
title={title} |
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.
Let's remove this attribute. Per #11520 we should avoid using the title
attribute.
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.
OK. Very interesting! Thanks!
ui_framework/components/tabs/tab.js
Outdated
}; | ||
|
||
KuiTab.propTypes = { | ||
title: PropTypes.string.isRequired, |
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.
We can remove it here too.
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.
Ok
ui_framework/components/tabs/tabs.js
Outdated
|
||
this.state = { | ||
selectedTabIx: this.getStartSelectedIx() | ||
}; |
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.
Let's just rely on the selectedTabIndex
prop as the source of truth instead of duplicating it internally.
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.
Also, we generally try to avoid using abbreviations like this, so we should write out index
instead of ix
.
ui_framework/components/tabs/tabs.js
Outdated
} else { | ||
return undefined; | ||
} | ||
} |
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.
We also won't need this helper, since the owner will be responsible for providing the initial index.
ui_framework/components/tabs/tabs.js
Outdated
if (nextProps.selectedTabIx && (this.state.selectedTabIx !== nextProps.selectedTabIx)) { | ||
this.setState({ selectedTabIx:nextProps.selectedTabIx }); | ||
} | ||
} |
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.
We'll also be able to remove this.
@@ -0,0 +1,24 @@ | |||
import React from 'react'; |
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.
Since we're using this approach in the original KuiTabs
component, we can delete this file.
@@ -0,0 +1,17 @@ | |||
/* eslint-disable */ |
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.
We can remove this example.
@@ -11,6 +12,14 @@ import { | |||
const html = require('./tabs.html'); | |||
const js = require('raw!./tabs.js'); |
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.
Would you mind deleting the original example files and removing the example from this file?
<KuiTab title="Monosodium Glutamate">Monosodium Glutamate</KuiTab> | ||
]} | ||
onSelectedTabChanged={(tabIx,tabTitle)=>console.log(tabIx,tabTitle)} | ||
/>; |
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.
Let's remove this example, since I don't think it will be very useful for most people's use cases.
@@ -33,5 +42,47 @@ export default props => ( | |||
js={js} | |||
/> | |||
</GuideSection> | |||
|
|||
<GuideSection |
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.
We'll need a new example file which demonstrates just using the KuiTabs React component. I'm thinking it will look something like this:
constructor() {
this.tabs = [
'Cobalt',
'Dextrose',
'Helium-3',
'Monosodium Glutamate',
];
this.state = {
selectedTabIndex: 0,
};
}
onSelectedTabChanged(index) {
this.setState({
selectedTabIndex: index,
});
}
render() {
<KuiTabs
selectedTabIndex={this.state.selectedTabIndex}
onSelectedTabChanged={this.onSelectedTabChanged}
>
{this.tabs}
</KuiTabs>
}
@cjcenizal This is exciting! Thanks! I had worried about the selectedTabIndex prop as start state, but I had no better idea. Passing that responsibility to the owner is the React way. Tests are coming. |
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.
🍾 Beautiful! Just had a few really minor suggestions and then we're looking really good!
ui_framework/components/tabs/tabs.js
Outdated
}) => { | ||
const classes = classNames('kuiTabs', className); | ||
const tabs = children.map((child,index)=>( | ||
<KuiTab |
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.
Looks like this JSX needs to be outdented once.
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.
Sorry...
ui_framework/components/tabs/tabs.js
Outdated
return new Error(`${componentName}'s selectedTabIndex must be undefined if there is no tab to select.`); | ||
} | ||
} | ||
}; |
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 refactor this to use the "early exit" pattern, to simplify the conditions we're testing against?
// An undefined selected tab is OK in all situations.
if (selectedTabIndex === undefined) {
return;
}
if (childrenLength === 0) {
throw new Error(`${componentName}'s selectedTabIndex must be undefined if there is no tab to select.`);
}
if (selectedTabIndex < 0 || selectedTabIndex > childrenLength -1) {
throw new Error(`${componentName}'s selectedTabIndex must be within the range defined by the number of tabs.`);
}
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.
Could you also add tests to assert that these errors are thrown under the defined conditions? Take a look at https://github.com/elastic/kibana/blob/master/ui_framework/components/accessibility/keyboard_accessible.test.js#L16 to see the pattern we've used for defining these kinds of tests.
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.
Much better syntax for selectedTabIndexCheck! I make the tests.
selectedTabIndex={0} | ||
onSelectedTabChanged={()=>{}} | ||
{ ...requiredProps } | ||
> |
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.
I think this >
needs to be outdented by one space?
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.
You see it right :) OK.
<KuiTabs | ||
selectedTabIndex={0} | ||
onSelectedTabChanged={onSelectedTabChangedHandler} | ||
> |
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.
Same here.
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.
Ok
<KuiTabs | ||
selectedTabIndex={0} | ||
onSelectedTabChanged={onSelectedTabChangedHandler} | ||
> |
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.
And here.
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.
Ok
}]} | ||
> | ||
<GuideText> | ||
Wrap any series of components, e.g. Panel, in the VerticalRhythm component to space | ||
them apart. | ||
The children of the KuiTabs can be anything that can be rendered: they will be wrapped into KuiTab components. |
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 tweak this slightly?
The <GuideCode>KuiTabs</GuideCode> component accepts nodes as children. Each child will be wrapped within a <GuideCode>KuiTab</GuideCode> component.
You'll just need to import GuideCode
from ../../components
on line 10.
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.
It sounds much better.
@cjcenizal Thanks for your suggestions and examples! |
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.
Superb!!! This looks great @PopradiArpad.
ui_framework/components/tabs/tab.js
Outdated
isSelected: PropTypes.bool, | ||
onClick: PropTypes.func, | ||
children: PropTypes.node, | ||
className: React.PropTypes.string |
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.
Remove unneeded React prefix
ui_framework/components/tabs/tabs.js
Outdated
children: PropTypes.node, | ||
selectedTabIndex: selectedTabIndexCheck, | ||
onSelectedTabChanged: PropTypes.func.isRequired, | ||
className: React.PropTypes.string |
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.
Remove unneeded React prefix
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.
Thank you for yet another nice set of additions to the KUI framework, @PopradiArpad!
I know that I'm late to the discussion, so you probably already weighed the ups and downs of this API with @cjcenizal. I'm wondering whether the slightly reduced boilerplate (compared to providing <KuiTab>
children explicitly) is worth these downsides:
- The user cannot provide custom classes on the tabs, potentially forcing her to use non-BEM nested selectors to style them.
- The
selectedTabIndex
prop assumes that the user represents the state using an integer. I myself would usually use identifier strings for that in my state to be order-independent and more readable.
ui_framework/components/tabs/tab.js
Outdated
onClick: PropTypes.func, | ||
children: PropTypes.node, | ||
className: PropTypes.string | ||
}; |
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.
I think we want to provide defaultProps
for all non-required props.
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.
onClick should be required: a tab is to be selected and not only to represent something.
The default of isSelected should be false.
@weltenwort What should be the default of children? The string "tab" or "undefined" ?
className has no defaultProps in other components either: it's really optional.
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.
I think the "official" default for children
is null
, which results in no children being rendered.
ui_framework/components/tabs/tabs.js
Outdated
...rest | ||
}) => { | ||
const classes = classNames('kuiTabs', className); | ||
const tabs = children.map((child,index)=>( |
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.
The linter should have complained about the missing space after the comma. 🤔
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.
@PopradiArpad Could you check that you have the latest npm dependencies installed? Deleting node_modules
and running npm install
will ensure everything is up to date too. The linter has been falling out of date for some people recently.
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.
Thanks!
ui_framework/components/tabs/tabs.js
Outdated
selectedTabIndex: selectedTabIndexCheck, | ||
onSelectedTabChanged: PropTypes.func.isRequired, | ||
className: PropTypes.string | ||
}; |
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.
These could also benefit from defaultProps
.
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.
className has no defaultProps in other components either: it's really optional.
ui_framework/components/tabs/tabs.js
Outdated
|
||
const selectedTabIndexCheck = (props, propName, componentName) => { | ||
const childrenLength = props.children ? props.children.length : 0; | ||
const selectedTabIndex = props.selectedTabIndex; |
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.
How about using the propName
argument instead of hardcoding selectedTabIndex
? Maybe we could even parameterize the use of children
as in
const propIndex = (indexedPropName) => (props, propName, componentName) => {
const indexedPropLength = props[indexedPropName] ? props[indexedPropName].length : 0;
const propValue = props[propName];
// ...
};
KuiTabs.propTypes = {
// ...
selectedTabIndex: propIndex('children'),
// ...
};
And maybe we want to include a type check for Number
as well?
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.
The function arguments are given by React and says: let check this property in
the context of all properties. I think we can assume that the user of this lib know
this feature of React. If this a problem than probably some general documentation would
be better than using React other than recommended.
I add a check for selectedTabIndex: it must be an integer.
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.
But writing it in a way that doesn't hard-code the property name is trivial. Why not do it?
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.
Aha. You think about re-use. I would do it in a later run when there are enough patterns to abstract.
Until that better so simple to read as possible and that means staying so close to React itself as possible.
); | ||
}); | ||
|
||
test(`when selectedTabIndex is greater then the upper limit defined by the number of tabs`, () => { |
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.
"then" => "than"
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.
Thanks!
'Monosodium Glutamate', | ||
]; | ||
|
||
describe('throws an error', () => { |
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.
I feel like these tests should just test the selectedTabIndexCheck
function itself without being attached to a component.
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.
@weltenwort We have similar error-related tests in https://github.com/elastic/kibana/blob/master/ui_framework/components/accessibility/keyboard_accessible.test.js#L16, where we could also implement your suggestion.
Before we move forward with your suggestion, I think we should define best practices for defining propType functions and associating them with components. Should we export them from their own module? Should they live within the same directory as the component which uses them? Should we group them together in a prop_types
directory?
For now, I think we should keep these tests as they are, for the sake of simplicity and consistency, and then refactor all of the propType functions once we've defined best practices. What do you think?
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.
Intuitively, I would say: If they can be written for re-use, a library of prop-types sounds like a good idea. If they are absolutely component-specific (which should be pretty rare, I can't think of such a case right now) they should live with the component (maybe in a local prop_types.js
).
Stubbing console
to test them feels really crude, since they are supposed to be pure functions. But if there are other instances of this, then I would agree that they could be cleanup up later in one go.
|
||
describe('Props', () => { | ||
describe('onClick', () => { | ||
test(`isn't called upon instantiation`, () => { |
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.
Is that something that needs to be tested? We do not test for all other things that do not happen on instantiation either.
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.
Good point, I think we can remove 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.
I took this kind of test from button, but I don't see it relevant. I remove it even from tabs.test.js.
selectedTabIndex: 0, | ||
}; | ||
|
||
this.onSelectedTabChanged = this.onSelectedTabChanged.bind(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.
We can get rid of this if we define onSelectedTabChanged
like that:
onSelectedTabChanged = (index) => {
// ...
}
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.
Super! Thanks!
@weltenwort I like this idea. What do you think of creating three components: generic The existing import React from 'react';
import PropTypes from 'prop-types';
import classNames from 'classnames';
import { KuiTab } from './tab';
export const KuiTabGroup = ({
children,
className,
...rest
}) => {
const classes = classNames('KuiTabGroup', className);
return (
<div
className={classes}
{...rest}
>
{children}
</div>
);
};
/**
* children: Each element of this array will be wrapped into KuiTab
* onSelectedTabChanged: Arguments: tabIndex
*/
KuiTabGroup.propTypes = {
children: PropTypes.node,
className: PropTypes.string
}; And import React from 'react';
import PropTypes from 'prop-types';
import { KuiTab } from './tab';
import { KuiTabGroup } from './tab_group';
export const KuiTabs = ({
children,
selectedTabIndex,
onSelectedTabChanged,
...rest
}) => {
const tabs = children.map((child,index)=>(
<KuiTab
key={index}
onClick={() => onSelectedTabChanged(index)}
isSelected={index === selectedTabIndex}
>
{child}
</KuiTab>
));
return (
<KuiTabGroup
{...rest}
>
{tabs}
</KuiTabGroup>
);
};
const selectedTabIndexCheck = (props, propName, componentName) => {
const childrenLength = props.children ? props.children.length : 0;
const selectedTabIndex = props.selectedTabIndex;
// An undefined selected tab is OK in all situations.
if (selectedTabIndex === undefined) {
return;
}
if (childrenLength === 0) {
throw new Error(`${componentName}'s selectedTabIndex must be undefined if there is no tab to select.`);
}
if ((selectedTabIndex < 0) || (selectedTabIndex > (childrenLength - 1))) {
throw new Error(`${componentName}'s selectedTabIndex(${selectedTabIndex}) must be within the range defined by the number of tabs.`);
}
};
/**
* children: Each element of this array will be wrapped into KuiTab
* onSelectedTabChanged: Arguments: tabIndex
*/
KuiTabs.propTypes = {
...KuiTabGroup.propTypes,
selectedTab: selectedTabIndexCheck,
onSelectedTabChanged: PropTypes.func.isRequired,
}; This way people can have the option of using the simplified interface, or customizing their instance of
I'm not sure I understand what you mean by using identifier strings. How do we use this identifier to determine which tab is selected? Can you show us how to implement this? Thanks man! Note: I've edited this comment a couple times. I changed the names of the suggested components to adhere more closely to the pattern defined by our "Button" and "ButtonGroup" components. Note that this change will require updating Kibana to use the changed class names. |
@PopradiArpad I met up with @weltenwort and we decided that we should simplify the import React from 'react';
import PropTypes from 'prop-types';
import classNames from 'classnames';
export const KuiTabs = ({
children,
className,
...rest
}) => {
const classes = classNames('kuiTabs', className);
return (
<div
className={classes}
{...rest}
>
{children}
</div>
);
};
KuiTabs.propTypes = {
children: PropTypes.node,
className: PropTypes.string
}; We can update the example to contain similar logic to what we have now, but without the error-checking: import React from 'react';
import {
KuiTab,
KuiTabs,
} from '../../../../components';
class KuiTabsExample extends React.Component {
constructor(props) {
super(props);
this.tabs = [{
id: 'cobalt',
name: 'Cobalt',
}, {
id: 'dextrose',
name: 'Dextrose',
}, {
id: 'helium_3',
name: 'Helium-3',
}, {
id: 'monosodium_glutammate',
name: 'Monosodium Glutamate',
}];
this.state = {
selectedTabId: 'cobalt',
};
}
onSelectedTabChanged = id => {
this.setState({
selectedTabId: id,
});
}
renderTabs() {
return this.tabs.map(tab => (
<KuiTab
onClick={() => this.onSelectedTabChanged(tab.id)}
isSelected={tab.id === this.state.selectedTabId}
>
{tab.name}
</KuiTab>
));
}
render() {
return (
<KuiTabs>
{this.renderTabs()}
</KuiTabs>
);
}
}
export default KuiTabsExample; At some point in the future, we can create a more opinionated component which composes Sorry about all of the churn on this! What do you think? |
Even if this looks like churn, it forces us to make some important decisions for the future direction of the kui framework - which is a good thing. Thanks for breaking ground with this, @PopradiArpad! |
The above commit is not the restructuring. To the problem
It's true but she has all might to provide classes to the wrapped components. My opinion: make the structure simple, help the user and cover the obvious uses cases first. So as I see the new structure solve a not so relevant problem but make harder the basic use case. Of course, if you think the new structure fits better to Kibana, I make that commit :) |
I like this approach too, and I think this PR can just be the first step in that process. What I'd like to do is first merge the new structure we're proposing, and then start using it within Kibana. When we're doing that, I think we'll discover the obvious use cases, which will be a great opportunity to build a more opinionated Tabs component. The new opinionated component will do a lot of the things you mentioned, like dealing with |
* Setting the selectedTabIndex prop is the responsibility of the owner. * Wrapping the children into KuiTab's is the responsibility of KuiTabs.
dfa40f9
to
8b27a16
Compare
@weltenwort , @cjcenizal Ok. Sorry for the late answer. I adapted the document text too. @cjcenizal you will find surely a better one than mine :) |
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.
🏎 This is fantastic! I just have one small request.
you will find surely a better one than mine
If I had to speculate, I'd bet that we'll end up reintroducing the more opinionated component in a very similar form to the way you wrote it! But when we do we'll be able to use it in Kibana immediately... so we'll be confident that it's designed well, because it will be tested in a real-world use case. 😄
ui_framework/components/index.js
Outdated
export { | ||
KuiPager, | ||
KuiPagerButtonGroup, | ||
} from './pager'; |
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.
Would you mind moving these exports back to lines 85 and 88? I did some reordering in master
which is probably what caused this conflict. 😄
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.
Ok.
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.
Thanks @PopradiArpad!!
That was fast! |
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.
Awesome, thank you!
@cjcenizal @weltenwort Thank you for your reviews and suggestions! |
Implement
and theirs test.
Update docs examples.