[Timeline] Introduce new component #21331
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
@material-ui/lab: parsed: +2.56% , gzip: +1.49% Details of bundle changes.Comparing: 1f18002...a7c8b3f Details of page changes
|
mnajdova
commented
Jun 6, 2020
packages/material-ui-lab/src/TimelineItemDot/TimelineItemDot.js
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Great start!
- We need to update https://github.com/mui-org/material-ui/blob/master/packages/material-ui-lab/src/index.d.ts
- We need to update https://github.com/mui-org/material-ui/blob/master/packages/material-ui-lab/src/index.js
- I think that we could have a dedicated demo for changing the color. https://ant.design/components/timeline/#components-timeline-demo-color
- It would be great to have a h2 section in the documentation for the dot variant outlined (pretty cool)
packages/material-ui-lab/src/TimelineItemDot/TimelineItemDot.js
Outdated
Show resolved
Hide resolved
packages/material-ui-lab/src/TimelineItemDot/TimelineItemDot.js
Outdated
Show resolved
Hide resolved
packages/material-ui-lab/src/TimelineItemDot/TimelineItemDot.js
Outdated
Show resolved
Hide resolved
packages/material-ui-lab/src/TimelineItemDot/TimelineItemDot.js
Outdated
Show resolved
Hide resolved
packages/material-ui-lab/src/TimelineItemDot/TimelineItemDot.js
Outdated
Show resolved
Hide resolved
packages/material-ui-lab/src/TimelineItemDot/TimelineItemDot.js
Outdated
Show resolved
Hide resolved
oliviertassinari
added
component: timeline
This comment has been minimized.
This comment has been minimized.
mbrookes
previously requested changes
Jun 6, 2020
packages/material-ui-lab/src/TimelineItemDot/TimelineItemDot.js
Outdated
Show resolved
Hide resolved
packages/material-ui-lab/src/TimelineItemDot/TimelineItemDot.js
Outdated
Show resolved
Hide resolved
|
A new thought, should we make the component names "denser", turning <Timeline>
<TimelineItem>
<TimelineItemTail />
<TimelineItemDot />
<TimelineItemContent>Code</TimelineItemContent>
</TimelineItem>
</Timeline>into? <Timeline>
<TimelineItem>
<TimelineTail />
<TimelineDot />
<TimelineContent>Code</TimelineContent>
</TimelineItem>
</Timeline> |
|
@oliviertassinari here is how the API looks at the moment - I have denser component names, and additional component around the <TimelineItem>
<TimelineSeparator>
<TimelineDot />
<TimelineConnector />
</TimelineSeparator>
<TimelineContent>Code</TimelineContent>
</TimelineItem> |
mbrookes
reviewed
Jun 7, 2020
mbrookes
reviewed
Jun 7, 2020
oliviertassinari
approved these changes
Jun 8, 2020
joshwooding
reviewed
Jun 8, 2020
eps1lon
approved these changes
Jun 9, 2020
There was a problem hiding this comment.
I really like this API. Especially since you didn't start with a high level API where you just pass in some items to a single component. Instead you start with multiple components that are composed into one rich UI. And the demos then show how one could abstract the data into a single variable (e.g. in "Opposite content").
My only issue is that all components have a component prop. Since we don't have any documented use case I'd remove it for now. Especially since the default has semantics (e.g. ul in Timeline). It's always easier to add a prop later or maybe find a better implementation for a specific use case than removing a prop because it's used wrong.
What's going to be interesting is how accessibility looks for this component.
Either way great work!
| Here is an example of customizing the component. You can learn more about this in the | ||
| [overrides documentation page](/customization/components/). | ||
|
|
||
| {{"demo": "pages/components/timeline/CustomizedTimeline.js"}} |
There was a problem hiding this comment.
I'm leaving a not for later, so far, in the ## Customized X headers of the documentation, we have always tried to show an example that significantly diverges from Material Design. It could potentiation make sense to follow suit here. but IMHO, it's already good enough right now to be merged like this :)
@eps1lon thanks for the review, removed component props. |
|
Noticed this on a smaller screen: Looks like it happens due to the limited space available with all 3 next to each other - demo displays weirdly on mobile too. |
joshwooding
reviewed
Jun 9, 2020
joshwooding
reviewed
Jun 9, 2020
| // | To update them edit the d.ts file and run "yarn proptypes" | | ||
| // ---------------------------------------------------------------------- | ||
| /** | ||
| * The position where the timeline should appear. |
There was a problem hiding this comment.
This is a bit confusing for me. The timeline is being referred to as the visible line in the documentation so how can it have alternating alignment? I think this is what caused me some initial confusion when looking at the right aligned timeline example - I instantly associated alignment with text so when the text was aligned on the left it seemed weird until I read the description. Maybe something like eventAlignment= left | right | alternating would be better, I'm not sure.
This to me doesn't have to be changed straight away or maybe at all just something I noticed :)
There was a problem hiding this comment.
I got the inspiration for this prop from other UI libraries actually...
- ant design has the property
mode="right"which is displaying the same thing - react suite also has the align property -
align="right"for the similar thing
Maybe eventAlignment is more specific and won't introduce this confusion... Anyway, let me know you final thoughts and I can update it.
There was a problem hiding this comment.
@joshwooding From my perspective, we are good. If we consider how the CSS text-align property work, it sounds identical.
There was a problem hiding this comment.
@oliviertassinari I’m not sure I quite understand your point my point was changing the alignment of the timeline to alternating makes no sense when referring to the visible line as the timeline? text-align specifies in the name what is being aligned? Alternating text alignment makes sense to me alternating line alignment doesn’t...
@mnajdova
Mode makes more sense in this context but I prefer alignment better as a concept, react-suite calls it align but the prop description is different so maybe that’s the middle ground in keeping the prop name and clearing up the meaning?
There was a problem hiding this comment.
text-align specifies in the name what is being aligned
Interesting, I guess we might have abused on the shorthand with TableCell.align and Typography.align prop names.
Suggested change
| * The position where the timeline should appear. | |
| * The position where the timeline's content should appear. |
@joshwooding I updated that example to only contain the alternating Timeline, checked on mobile, looks good now.. |
Co-authored-by: Matt <github@nospam.33m.co>
Co-authored-by: Matt <github@nospam.33m.co>
Co-authored-by: Olivier Tassinari <olivier.tassinari@gmail.com>
Co-authored-by: Olivier Tassinari <olivier.tassinari@gmail.com>
Co-authored-by: Olivier Tassinari <olivier.tassinari@gmail.com>
Co-authored-by: Olivier Tassinari <olivier.tassinari@gmail.com>
Co-authored-by: Sebastian Silbermann <silbermann.sebastian@gmail.com>
oliviertassinari
force-pushed
the
feat/add-timeline
branch
from
cf71bba
to
4b9f95d
Compare
Co-authored-by: Josh Wooding <12938082+joshwooding@users.noreply.github.com>
joshwooding
reviewed
Jun 10, 2020
|
@mnajdova Great job 🙏 |
|
@mnajdova Great work 🎉 |
|
wow, that is cool, it's just what I needed to my webapp, but had to develop it by myself see it's use case: HERE |
|
Nice job. Gives me a few potential enhancement ideas for this one...
|
2 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Documentation page: https://deploy-preview-21331--material-ui.netlify.app/components/timeline/