-
-
Notifications
You must be signed in to change notification settings - Fork 2.4k
Refactor TimelineSeparator to shared-components #31797
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
base: develop
Are you sure you want to change the base?
Refactor TimelineSeparator to shared-components #31797
Conversation
…remove unused type export
|
|
florianduros
left a comment
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.
Good first PR!
The PR is missing the view model. MockedViewModel is only for testing. We should create new view model class for TimelineSeparator
packages/shared-components/src/message-body/TimelineSeparator/TimelineSeparatorView.tsx
Outdated
Show resolved
Hide resolved
packages/shared-components/src/message-body/TimelineSeparator/TimelineSeparatorView.stories.tsx
Outdated
Show resolved
Hide resolved
packages/shared-components/src/message-body/TimelineSeparator/TimelineSeparatorView.test.tsx
Outdated
Show resolved
Hide resolved
packages/shared-components/src/message-body/TimelineSeparator/TimelineSeparatorView.tsx
Outdated
Show resolved
Hide resolved
packages/shared-components/src/message-body/TimelineSeparator/TimelineSeparatorView.module.css
Outdated
Show resolved
Hide resolved
packages/shared-components/src/message-body/TimelineSeparator/index.ts
Outdated
Show resolved
Hide resolved
…index.ts Co-authored-by: Florian Duros <florian.duros@ormaz.fr>
…TimelineSeparatorView.module.css Co-authored-by: Florian Duros <florian.duros@ormaz.fr>
…TimelineSeparatorView.tsx Co-authored-by: Florian Duros <florian.duros@ormaz.fr>
…TimelineSeparatorView.test.tsx Co-authored-by: Florian Duros <florian.duros@ormaz.fr>
…TimelineSeparatorView.stories.tsx Co-authored-by: Florian Duros <florian.duros@ormaz.fr>
…TimelineSeparatorView.tsx Co-authored-by: Florian Duros <florian.duros@ormaz.fr>
…props, and enhance type definitions
| * TimelineSeparatorView component renders a visual separator inside the message timeline. | ||
| * It draws horizontal rules with an accessible label and optional children rendered between them. | ||
| * | ||
| * @param label the accessible label string describing the separator (used for `aria-label`) | ||
| * @param children optional React nodes to render between the separators | ||
| * | ||
| */ |
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.
@florianduros Does this look better? If not, what do you suggest?
|
|
||
| .timelineSeparator { | ||
| clear: both; | ||
| margin: var(--cpdSpace1X, var(--cpdSpace0X)); |
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.
Like this?
packages/shared-components/src/message-body/TimelineSeparator/TimelineSeparatorView.tsx
Outdated
Show resolved
Hide resolved
… type definition of children
| /** | ||
| * Optional children to render inside the timeline separator | ||
| */ | ||
| children?: PropsWithChildren["children"]; |
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.
Would it be fine using ["children"];
|
|
||
| // Keep mx_TimelineSeparator to support the compatibility with existing timeline and the all the layout | ||
| return ( | ||
| <div className={classNames("mx_TimelineSeparator", styles.timelineSeparator)} role="separator" aria-label={label}> |
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.
{classNames("mx_TimelineSeparator", styles.timelineSeparator)}
I would like to hear your opinion regarding this, this is like to comment says, to "Keep mx_TimelineSeparator to support the compatibility with existing timeline and the all the layout"
…Children for improved type safety of children
| interface TimeLineSeperatorBodyWrapper { | ||
| label: string; | ||
| children?: ReactNode; | ||
| } | ||
|
|
||
| function TimeLineSeperatorBodyWrapper({ label, children }: TimeLineSeperatorBodyWrapper): JSX.Element { | ||
| const dateSeperatorVM = useCreateAutoDisposedViewModel(() => new TimelineSeparatorViewModel({ label, children })); | ||
| return <TimelineSeparatorView vm={dateSeperatorVM} />; | ||
| } |
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.
Can these be placed at the bottom? I found a similar file where these were located at the bottom, which is why I placed them there. I suggest moving them up and adding a comment.
Checklist
public/exportedsymbols have accurate TSDoc documentation.Refactor TimelineSeparator to shared-components
Summary
TimelineSeparatorcomponent to@element-hq/web-shared-componentspackage asTimelineSeparatorViewTimelineSeparatorViewSnapshotinterfaceTimelineSeparatortoMessagePanel.tsxChanges
New shared component
Added
TimelineSeparatorViewunderpackages/shared-components/src/message-body/TimelineSeparator/with:useViewModelhookCodebase migration
MessagePanel.tsx: Updated late event separator to useTimelineSeparatorViewDateSeparator.tsx: Now rendersTimelineSeparatorViewinstead of the old componentCreationGrouper.tsx/MainGrouper.tsx: UpdatedSeparatorKindimports to useMessagePanel.tsxTest plan
npm test -- --testPathPatterns="TimelineSeparatorView" -uTests
Before & After