Initial commit for accordian #31
Conversation
| @@ -0,0 +1,62 @@ | |||
| // @flow | |||
There was a problem hiding this comment.
Accordian generally works in group; current implementation doesn't handle this. see https://www.w3.org/TR/wai-aria-practices/examples/accordion/accordion.html for an example; you can also explore deatil and summary html5 elements for the same.
| } | ||
|
|
||
| const AccordianHeader = ({ className, onClick, children }: Props): Node => ( | ||
| <React.Fragment> |
There was a problem hiding this comment.
Redundant usage of Fragment. below as well
| import styles from './Accordian.style'; | ||
| import type { Props, State } from './types'; | ||
|
|
||
| class Accordian extends PureComponent<Props, State> { |
There was a problem hiding this comment.
@DipshiGoyal i would expect you to built this component like this https://codepen.io/amitmojumder/pen/xOKjPW
There was a problem hiding this comment.
The suggestion has been incorporated.
|
|
||
| import Accordian from '../index'; | ||
|
|
||
| describe('<Accordian />', () => { |
There was a problem hiding this comment.
The test needs to be much better, you need to test the expend collapse functionality
| margin: 10px 0; | ||
| `; | ||
|
|
||
| const AccordianHeader = styled.div` |
There was a problem hiding this comment.
This is not correct style of applying CSS, you can follow same BEM style in styled components too.
|
|
||
| return ( | ||
| <AccordianWrapper> | ||
| <AccordianHeader |
There was a problem hiding this comment.
AccordianHeader will not be DIV but a summary tag, you should built it as separate component. Same applies to Detail too. You can export them same as accordion and people can loop over them and build accordion groups.
|
@DipshiGoyal lets connect today, i feel there is some gap in our understanding for this component. |
3 participants
@vinodloha ,as disccussed I have added the functionality of accordian. Please review.
PFA
accordian (2).zip