feat: improve ActivityTimeline and TimelineMarker components
#2100
Conversation
Features
Tests
Bug Fixes
Documentation
Styles
ContributorsCommit-Lint commandsYou can trigger Commit-Lint actions by commenting on this PR:
|
![codeclimate[bot]](https://avatars.githubusercontent.com/in/9403?s=60&v=4){kind=small}
|
Visit the preview URL for this PR (updated for commit 4476552): https://react-rainbow--pr2100-feat-add-variant-pro-mrcg8pnw.web.app (expires Fri, 22 Jan 2021 23:06:11 GMT) 🔥 via Firebase Hosting GitHub Action 🌎 |
|
@wildergd let's use a |
|
@wildergd we need to fix the line color, in some cases it is not visible, I think we should use here an alpha color that guarantees that it is darker than the background |
…ub.com/90milesbridge/react-rainbow into feat-add-variant-prop-activitytimeline
|
Code Climate has analyzed commit 4476552 and detected 15 issues on this pull request. Here's the issue category breakdown:
View more on Code Climate. |
| export interface ActivityTimelineProps extends BaseProps { | ||
| children?: ReactNode; | ||
| multiple?: boolean; | ||
| onToggleSection?: (event: MouseEvent<HTMLElement>, name: Names) => void; |
There was a problem hiding this comment.
Here we will pass one single argument, a object with the name:
{ name }
| <StyledUl className={className} style={style}> | ||
| {children} | ||
| <StyledUl id={id} className={className} style={style} ref={containerRef} variant={variant}> | ||
| <Provider value={context}>{children}</Provider> | ||
| </StyledUl> |
There was a problem hiding this comment.
I think here, we should execute all this new code only if the variant is accordion:
if (variant === 'accordion') {
return <NewAccordionComponent />;
}
return <OriginalComponent />;
| PropTypes.string, | ||
| ]), | ||
| /** Action fired when a TimelineMarker is selected. | ||
| * The event params include the `name` of the selected TimelineMarker. */ |
There was a problem hiding this comment.
we need to explain that this props (multiple, activeSectionNames and onToggleSection) are only used for accordion variant
| style, | ||
| } = props; | ||
| const { | ||
| isVariantAccordion, |
There was a problem hiding this comment.
I think we should pass variant, since if we add new ones in the future then is more easy to make things here based on the variant
|
|
||
| return ( | ||
| <StyledLi className={className} style={style}> | ||
| <StyledLi data-id="timeline-marker-li" className={className} style={style}> |
There was a problem hiding this comment.
same ^^ here we should only execute all this new code if it is accordion variant
| @@ -9,7 +12,7 @@ const StyledColumnLeft = attachThemeAttrs(styled.div)` | |||
|
|
|||
| ::before { | |||
| content: ''; | |||
| background-color: ${props => props.palette.background.highlight}; | |||
| background-color: ${props => getColor(props)}; | |||
There was a problem hiding this comment.
can pass getColor directly
LeandroTorresSicilia
deleted the
feat-add-variant-prop-activitytimeline
branch
…xtway#2100) * feat: add variant prop to ActivityTimeline component * test: fix TimelineMarker tests * fix: lint issues * docs: review activitytimeline component info * docs: add info to activitytimeline examples * docs: add more info to activitytimeline component * style: fix style for timelinemarker guide * docs: add info to timelinemarker component * docs: fix typos in activitytimeline info * docs: add an article to activitytimeline example * docs: fix example * docs: add description to timelinemarker examples Co-authored-by: YulyGP <71304589+YulyGP@users.noreply.github.com>
fix: #2099
Changes proposed in this PR:
add
variantprop toActivityTimelinecomponentadd
isLoadingprop to TimelineMarker componentI have followed (at least) the PR section of the contributing guide.
@nexxtway/react-rainbow