-
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] Reactify kuiCollapseButton #12225
[UI Framework] Reactify kuiCollapseButton #12225
Conversation
Can one of the admins verify this patch? |
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 couple small suggestions.
KuiCollapseButton.propTypes = { | ||
className: PropTypes.string, | ||
direction: PropTypes.oneOf(DIRECTIONS).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.
I think this is a case where we can export an object, like in the button.js
file:
export {
DIRECTIONS,
KuiCollapseButton,
};
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 are right, DIRECTIONS is part of the interface, even if it's not used directly by the user.
Ok.
Probably later if the UI framework is more stable this interface can be extended/replaced by named collapse buttons like KuiCollapseButtonUp
(the same idea as I said about KuiEventSymbol
). It's a shorter syntax but not so flexible than this.
describe('KuiCollapseButton', () => { | ||
describe('Props', () => { | ||
describe('direction', () => { | ||
['up','down','left','right'].forEach(direction => { |
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 then we can import DIRECTIONS
above, and loop through them here (note that I also changed the test
name):
DIRECTIONS.forEach(direction => {
describe(direction, () => {
test(`renders the ${direction} class`, () => {
const component = <KuiCollapseButton direction={direction} { ...requiredProps }/>;
expect(render(component)).toMatchSnapshot();
});
});
});
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
|
||
export const KuiCollapseButton = ({ className, direction, ...rest }) => { | ||
const classes = classNames('kuiCollapseButton', className); | ||
const childClasses = classNames('kuiIcon', `fa-chevron-circle-${direction}`); |
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 create a special map here, so it's easier to grep for the specific class names we're using? This is similar to the pattern used in button.js
.
import React, {
PropTypes,
} from 'react';
import classNames from 'classnames';
const DIRECTIONS = [
'down',
'up',
'left',
'right',
];
const directionToClassNameMap = {
down: 'fa-chevron-circle-down',
up: 'fa-chevron-circle-up',
left: 'fa-chevron-circle-left',
right: 'fa-chevron-circle-right',
};
export const KuiCollapseButton = ({ className, direction, ...rest }) => {
const classes = classNames('kuiCollapseButton', className);
const childClasses = classNames('kuiIcon', direction[direction]);
return (<button
type="button"
className={classes}
{...rest}
>
<div className={childClasses}/>
</button>);
};
KuiCollapseButton.propTypes = {
className: PropTypes.string,
direction: PropTypes.oneOf(DIRECTIONS).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.
Ok. See next comment.
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.
This is fantastic! Thanks @PopradiArpad.
@cjcenizal Thanks for review and merge! |
Feature
KuiCollapseButton React component with tests.