[UI] Implement timeline component #3594
Conversation
|
@yaooluu, you must sign every commit in this pull request acknowledging our Developer Certificate of Origin before your changes are merged. This can be done by adding
|
|
Deploy preview for vmware-clarity ready! Built with commit c63e062 |
|
@colinreedmiller Here is the final implementation to review https://deploy-preview-3594--vmware-clarity.netlify.com/dev click on Timeline in sidenav. Do you approve? |
|
@gnomeontherun won't we need a website documentation page? Or we'll have a separate PR/ticket for it? |
colinreedmiller
left a comment
colinreedmiller
left a comment
There was a problem hiding this comment.
Visual design conforms to specification. Documentation question remains outstanding.
gnomeontherun
left a comment
gnomeontherun
left a comment
There was a problem hiding this comment.
The implementation looks good for this use case, but I have a couple of questions and asks. I think we have to figure out the limits of what content we can expect people to put into timeline, and what amount of flexibility they should expect. Any details on the original spec would help!
Thanks for this, I think its pretty solid overall with the one major question about position relative (which I am currently at a loss of an alternative).
|
|
||
| .clr-timeline-step { | ||
| display: flex; | ||
| position: relative; |
There was a problem hiding this comment.
This might be a problem, it will cause clipping on anything like a dropdown menu. Have you tried any other options that would get around this? @hippee-lee any additional insights based on popover work?
There was a problem hiding this comment.
I did some experiments and the horizontal timeline still works fine without step (relative) and step-body:before (absolute).
But if I remove those, the vertical timeline doesn't work. The "line" is not displayed at all between steps. Or with only step-body::before (absolution) it won't be able to get its parent's (timeline-step) height by using 100%, instead it get's the full height of the entire timeline..Not sure what should be the solution to get rid of it...
|
@gnomeontherun the original spec had no dark theme design. My work has been to improve the visual design such that it was clearer - the decisions about what content to support were part of the original design. We need to look at use cases since from what I understand, this component is not widely used in VMWare products and its original need has been superseded by additional datagrid functionality. |
|
@yaooluu, you must sign every commit in this pull request acknowledging our Developer Certificate of Origin before your changes are merged. This can be done by adding
|
|
@gnomeontherun Could you review the above changes again and let me know your thoughts? |
|
I asked @lil-kim to take a look at the design spec and add the required callouts for dark theme so we have this available while its in flight. Figma file here: https://www.figma.com/file/l7MoP6qowUUqMgNVWnxS4g/Timeline-component-Static-3199?node-id=12%3A2030 |
|
I'm going to review again, wasn't sure if you were done. @colinreedmiller I think we can handle the dark theme version of this separately to help close the loop on this PR. |
I addressed most comments except for the And I thought it is for reviewer to "resolve" issue so I didn't close them. |
|
@gnomeontherun - Yes. I am completely finished with Light theme and its approved - implementation is correct. I included the dark theme callouts in case it was preferable to build it at the same time but later is fine also. Closing this PR without dark theme is fine with me. |
|
@yaooluu Can you squash your commits and update gemini tests? Since you added something to the dev app some of the images that include the nav get flagged. |
|
Also make sure your commit use signed-off-by! |
|
Just following up on this @yaooluu do you have an ETA on getting the PR fixed up? |
Yes I want to get this done before end of week. I actually have some questions regarding squash, rebase, and the |
|
@yaooluu, you must sign every commit in this pull request acknowledging our Developer Certificate of Origin before your changes are merged. This can be done by adding
|
|
@yaooluu, you must sign every commit in this pull request acknowledging our Developer Certificate of Origin before your changes are merged. This can be done by adding
|
This change implements a static timeline component using HTML/CSS, based on the design spec from Clarity team. Closes vmware-archive#3199 Signed-off-by: Yao Lu <ylu31@ncsu.edu>
|
@gnomeontherun I've squashed my commits and updated the gemini test. The pipeline is passing now! |
|
Thanks Yao! We'll get it reviewed and merged soon! |
8 participants
This change implements a static timeline component using HTML/CSS, based on the design spec from Clarity team.
Closes #3199
Signed-off-by: Yao Lu ylu31@ncsu.edu
Same code updated at: https://stackblitz.com/edit/clarity-timeline-demo-v3. Ready for reviewers to play with the code.
Question list:
spinnericon as a custom icon. How to achieve the same for the production? i.e. component consumer should not need to add this icon by themselves?