Feature 69 - ButtonsGroup [Dev] #81
Conversation
…omponent, letting the user provide diferent quanty of buttons and specific styles props
…d styling, added selectedStyles and unselectedStyles props of styling
…button will be true, and his documentations.
…, added his implementation and documentation
danielvaldivv
added
documentation
danielvaldivv
changed the title
danielvaldivv
added
feature
| import { PropTypes } from 'prop-types'; | ||
|
|
||
| const useStyleSheet = createStyleSheet( | ||
| (theme, { borderWrapperComponent }) => ({ |
There was a problem hiding this comment.
borderWrapperComponent? 🤔 this name makes me thinks that it is a component, but is a border radius unit right? maybe a name that specifies that? Like wrapperRadius?
| import { ButtonsGroup } from '@platzily-ui/components'; | ||
| import { useState, Fragment } from 'react'; | ||
|
|
||
| . |
There was a problem hiding this comment.
I dont get this points 🤔, what are we trying to communicate here?
| }); | ||
|
|
||
| ButtonsGroup.propTypes = { | ||
| actions: PropTypes.oneOfType([ |
There was a problem hiding this comment.
Shouldn't this prop type be different?
acions: PropTypes.arrayOf(PropTypes.shape({
childrenButton: PropTypes.element,
onClick: PropTypes.func,
selected: PropTypes.boolean,
})).isRequired|
|
||
| return ( | ||
| <div ref={ref} className={cx(classes.buttonsGroupWrapper, className)} {...otherProps}> | ||
| {state.map((button, index) => { |
There was a problem hiding this comment.
State needs to be an array always, so if the prop of actions is an object it will break.
| <button | ||
| type="button" | ||
| key={index} | ||
| onClick={() => { |
There was a problem hiding this comment.
why don't we abstract this to another function?
| if (selectedButtonDefault <= (state.length - 1)) { | ||
| setState(state.map((element, indexMap) => ( | ||
| (selectedButtonDefault === indexMap) | ||
| ? { |
There was a problem hiding this comment.
This ternary can be avoided if you return the object this way:
return {
...element,
selected: index === indexMap
}
|
|
||
| useEffect(() => { | ||
| if (selectedButtonDefault <= (state.length - 1)) { | ||
| setState(state.map((element, indexMap) => ( |
There was a problem hiding this comment.
This function does the same as setStateRender right?
| const cornerButtonsGroup = (index) => { | ||
| let styleButton; | ||
| if (index === 0) { styleButton = classes.firstButtonStyle; } | ||
| if (index === (stateLength - 1)) { styleButton = classes.lastButtonStyle; } |
There was a problem hiding this comment.
Not a fan of doing this haha.
I would prefer:
if (index === (stateLength - 1)) {
styleButton = classes.lastButtonStyle;
}| return separationStyle; | ||
| }; | ||
|
|
||
| function setStateRender(index) { |
There was a problem hiding this comment.
I think is a good thing abstrating the selected style with this function, but that meas that in the actions the user should not send the selected key anymore right? Why dont we remove it?
| import ButtonsGroup from './ButtonsGroup'; | ||
|
|
||
| describe('@Components/ButtonsGroup', () => { | ||
| it('Given the ButtonsGroup Component, when the props provide borderWrapperComponent with a number then the ButtonsGroup component will take border-radius attribute', () => { |
There was a problem hiding this comment.
Lets add more tests here, we could test that for example the buttons rendered are the same length as the actions. And the last clicked button has the selected styles.
| key={index} | ||
| onClick={() => { | ||
| setStateRender(index); | ||
| onClick(); |
There was a problem hiding this comment.
We needs to pass the event to the onClick prop, lets send also the index of the button so the user can identify it.
|
@danielvaldivv can we link the issue associated to this PR? |
|
Lets also fill the responsible please. |
edanfesi
added
the
carry-over-5
Checklist ✅
Button One clicked
Button Two clicked
Button Three clicked
resolves [#69]