-
Notifications
You must be signed in to change notification settings - Fork 11
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(Timeline): new component #1312
Conversation
Size stats
|
Accessibility report ℹ️ You can run this locally by executing |
Deploy preview for mistica-web ready! ✅ Preview Built with commit 4408737. |
src/timeline.tsx
Outdated
/> | ||
</svg> | ||
); | ||
} else if (asset.kind === 'number') { |
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.
[matter of taste] if all if
branches have its own return, I prefer to not use else
, so all if
s are in the same level
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.
I think an exhaustive switch is better here, changed
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.
Great job! :) I have a question: I can't seem to read the active event with the screen reader. I'm mentioning it just in case. Could there be an issue with this, or maybe I'm not doing it correctly?
}, | ||
]); | ||
|
||
export const asset = style({ |
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.
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.
fixed
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.
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.
it should be fixed now
|
||
type TimelineProps = { | ||
children?: React.ReactNode; | ||
orientation?: 'horizontal' | 'vertical'; |
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.
The reverse option in the direction would be missing
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.
What's the difference? can't you built the same without an additional reverse orientation?
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.
We need to remove the spacing between media and first element in naked card to avoid this problems. When media prop is undefined, this space should not be there
We also have to change the media property in naked card to optional.
https://tinyurl.com/24j4mhyp
don't know if this change can be make it in this pr or in other one
# [16.11.0](v16.10.0...v16.11.0) (2025-02-04) ### Features * **Timeline:** new component ([#1312](#1312)) ([03426af](03426af))
🎉 This PR is included in version 16.11.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
ticket: WEB-2125
specs: https://www.figma.com/design/X7I9t1mfcIHCZGuXozD8D5/Timeline-Specs?m=auto&node-id=58-4138&t=wywg4pcn2w717hnM-1