-
Notifications
You must be signed in to change notification settings - Fork 0
Feature 69 - ButtonsGroup [Dev] #81
base: develop
Are you sure you want to change the base?
Changes from all commits
dc50010
0b30fe4
4753c20
999ff18
7c45c58
c9cbcdd
c56baf8
625bcfe
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,5 @@ | ||
import { HTMLProps } from 'react'; | ||
|
||
export interface ButtonsGroupProps extends HTMLProps<HTMLButtonElement> {} | ||
|
||
export default function ButtonsGroup(props: ButtonsGroupProps): JSX.Element; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,165 @@ | ||
import { forwardRef, useEffect, useState } from 'react'; | ||
import { createStyleSheet } from '@platzily-ui/styling'; | ||
import { PropTypes } from 'prop-types'; | ||
|
||
const useStyleSheet = createStyleSheet( | ||
(theme, { borderWrapperComponent }) => ({ | ||
buttonsGroupWrapper: { | ||
width: 'auto', | ||
height: 'auto', | ||
display: 'inline-block', | ||
borderStyle: 'solid', | ||
borderColor: theme.palette.neutral.secondary, | ||
borderWidth: 1, | ||
borderRadius: borderWrapperComponent || theme.spacing(), | ||
}, | ||
firstButtonStyle:{ | ||
borderTopLeftRadius: borderWrapperComponent || theme.spacing(), | ||
borderBottomLeftRadius: borderWrapperComponent || theme.spacing(), | ||
}, | ||
lastButtonStyle:{ | ||
borderTopRightRadius: borderWrapperComponent || theme.spacing(), | ||
borderBottomRightRadius: borderWrapperComponent || theme.spacing(), | ||
}, | ||
separationLinesButton: { | ||
borderRightStyle: 'solid', | ||
borderRightColor: theme.palette.neutral.secondary, | ||
borderRightWidth: 1, | ||
}, | ||
buttonsStyles:{ | ||
padding: '5px 10px', | ||
}, | ||
buttonUnselected: { | ||
backgroundColor: theme.palette.neutral.light, | ||
color: theme.palette.neutral.dark | ||
}, | ||
buttonSelected: { | ||
backgroundColor: theme.palette.primary.main, | ||
color: theme.palette.neutral.light | ||
}, | ||
}), | ||
{ key: 'ButtonsGroup' }, | ||
); | ||
|
||
const ButtonsGroup = forwardRef(function ButtonsGroup(props, ref) { | ||
const { actions, className, classNameButtons, separationLinesButtonProp, selectedStyles, unselectedStyles, selectedButtonDefault, borderWrapperComponent, ...otherProps } = props; | ||
const [state, setState] = useState(actions); | ||
const { classes, cx } = useStyleSheet(props); | ||
const stateLength = actions.length; | ||
|
||
useEffect(() => { | ||
if (selectedButtonDefault <= (state.length - 1)) { | ||
setState(state.map((element, indexMap) => ( | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This function does the same as |
||
(selectedButtonDefault === indexMap) | ||
? { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This ternary can be avoided if you return the object this way: return { |
||
...element, | ||
selected: true, | ||
} | ||
: { | ||
...element, | ||
selected: false, | ||
} | ||
))); | ||
} else { | ||
setState(state.map((element, indexMap) => ( | ||
(indexMap === 0) | ||
? { | ||
...element, | ||
selected: true, | ||
} | ||
: { | ||
...element, | ||
selected: false, | ||
} | ||
))); | ||
} | ||
},[]); | ||
|
||
|
||
|
||
const selectButtonStyles = (stateButton) => { | ||
return stateButton ? (selectedStyles || classes.buttonSelected) : (unselectedStyles || classes.buttonUnselected); | ||
}; | ||
|
||
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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not a fan of doing this haha. I would prefer: if (index === (stateLength - 1)) {
styleButton = classes.lastButtonStyle;
} |
||
|
||
return styleButton; | ||
}; | ||
|
||
const separationLinesButton = (index) => { | ||
let separationStyle; | ||
if (index !== (stateLength - 1)) { separationStyle = separationLinesButtonProp || classes.separationLinesButton; } | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This condition can be in more than one line. |
||
|
||
return separationStyle; | ||
}; | ||
|
||
function setStateRender(index) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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? |
||
setState(state.map((element, indexMap) => ( | ||
(index === indexMap) | ||
? { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This ternary can be avoided if you return the object this way: return {
...element,
selected: index === indexMap
} |
||
...element, | ||
selected: true, | ||
} | ||
: { | ||
...element, | ||
selected: false, | ||
} | ||
))); | ||
}; | ||
|
||
return ( | ||
<div ref={ref} className={cx(classes.buttonsGroupWrapper, className)} {...otherProps}> | ||
{state.map((button, index) => { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. State needs to be an array always, so if the prop of actions is an object it will break. |
||
const { selected, childrenButton, onClick } = button; | ||
return ( | ||
<button | ||
type="button" | ||
key={index} | ||
onClick={() => { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. why don't we abstract this to another function? |
||
setStateRender(index); | ||
onClick(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We needs to pass the event to the onClick prop, lets send also the index of the button so the user can identify it. |
||
}} | ||
className={cx( | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We could use the object approach for these validations 🤔 |
||
selectButtonStyles(selected), | ||
cornerButtonsGroup(index), | ||
separationLinesButton(index), | ||
classes.buttonsStyles, | ||
classNameButtons | ||
)} | ||
> | ||
{childrenButton} | ||
</button> | ||
); | ||
})} | ||
</div> | ||
); | ||
}); | ||
|
||
ButtonsGroup.propTypes = { | ||
actions: PropTypes.oneOfType([ | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Shouldn't this prop type be different? acions: PropTypes.arrayOf(PropTypes.shape({
childrenButton: PropTypes.element,
onClick: PropTypes.func,
selected: PropTypes.boolean,
})).isRequired |
||
PropTypes.arrayOf(PropTypes.object).isRequired, | ||
PropTypes.shape({ | ||
childrenButton: PropTypes.element, | ||
onClick: PropTypes.func, | ||
selected: PropTypes.boolean, | ||
}).isRequired, | ||
]), | ||
borderWrapperComponent: PropTypes.number, | ||
className: PropTypes.string, | ||
classNameButtons: PropTypes.string, | ||
selectedButtonDefault: PropTypes.number, | ||
selectedStyles: PropTypes.string, | ||
separationLinesButtonProp: PropTypes.string, | ||
unselectedStyles: PropTypes.string, | ||
}; | ||
|
||
ButtonsGroup.defaultProps = { | ||
className: '', | ||
separationLinesButtonProp: undefined, | ||
|
||
}; | ||
|
||
export default ButtonsGroup; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,44 @@ | ||
import { render } from '@testing-library/react'; | ||
import { ThemeProvider, createTheme } from '@platzily-ui/styling'; | ||
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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
// arrange | ||
const actions = [ | ||
{ | ||
childrenButton: 'Button One', | ||
selected: false, | ||
onClick: () => {}, | ||
}, | ||
{ | ||
childrenButton: 'Button Two', | ||
selected: false, | ||
onClick: () => {}, | ||
}, | ||
{ | ||
childrenButton: 'Button Three', | ||
selected: false, | ||
onClick: () => {}, | ||
}, | ||
]; | ||
|
||
const { getByRole } = render( | ||
<ThemeProvider theme={createTheme()}> | ||
<ButtonsGroup | ||
role="banner" | ||
actions={actions} | ||
borderWrapperComponent={5} | ||
/> | ||
</ThemeProvider>, | ||
); | ||
|
||
// act | ||
const ButtonsGroupTestedText = getByRole('banner'); | ||
|
||
// assert | ||
expect(ButtonsGroupTestedText).toBeDefined(); | ||
expect(ButtonsGroupTestedText).toHaveStyle(`border-radius:5px`); | ||
}); | ||
|
||
}); |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,2 @@ | ||
export * from './ButtonsGroup'; | ||
export { default } from './ButtonsGroup'; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1 @@ | ||
export { default } from './ButtonsGroup'; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,50 @@ | ||
import { ButtonsGroup } from '@platzily-ui/components'; | ||
import { Fragment, useState } from 'react'; | ||
import { createStyleSheet } from '@platzily-ui/styling'; | ||
|
||
const useStyleSheet = createStyleSheet( | ||
(theme) => ({ | ||
buttonsGroupWrapper: { | ||
borderColor: theme.palette.neutral.secondary, | ||
}, | ||
}), | ||
{ key: 'ButtonsGroupImplementation' }, | ||
); | ||
|
||
export default function ButtonsGroupComponent(props) { | ||
const { classes } = useStyleSheet(props); | ||
|
||
const [render, setRender] = useState(''); | ||
|
||
const actions = [ | ||
{ | ||
childrenButton: 'Button One', | ||
selected: false, | ||
onClick: () => setRender('One'), | ||
}, | ||
{ | ||
childrenButton: 'Button Two', | ||
selected: false, | ||
onClick: () => setRender('Two'), | ||
}, | ||
{ | ||
childrenButton: 'Button Three', | ||
selected: false, | ||
onClick: () => setRender('Three'), | ||
}, | ||
]; | ||
|
||
return ( | ||
<Fragment > | ||
<span style={{ display:'flex', justifyContent:'center', margin:'20px' }}> | ||
<ButtonsGroup | ||
actions={actions} | ||
className={ classes.buttonsGroupWrapper } | ||
classNameButtons = { classes.ButtonsStyles } | ||
/> | ||
</span> | ||
<p style={{ textAlign:'center', margin:'20px' }}> Actions {render}</p> | ||
</Fragment> | ||
); | ||
} | ||
|
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.
borderWrapperComponent
? 🤔 this name makes me thinks that it is a component, but is a border radius unit right? maybe a name that specifies that? LikewrapperRadius
?