Skip to content

Initial structure for shared component views#30216

Merged
dbkr merged 60 commits into
developfrom
dbkr/textualevent_sharedcomponent
Jul 14, 2025
Merged

Initial structure for shared component views#30216
dbkr merged 60 commits into
developfrom
dbkr/textualevent_sharedcomponent

Conversation

@dbkr
Copy link
Copy Markdown
Member

@dbkr dbkr commented Jun 26, 2025

  • Turn the trivial TextualEvent into a shared component with a separate view model for element web.
  • Adds jest based unit testing for the shared components
  • Adds storybook for the shared components from @florianduros 's PR
  • Adds storybook / test-runner / playwright screenshot testing for each story

Not included in this PR (it's getting large...):

  • @florianduros 's shared component translation stuff
  • Way to use element-web-playwright-common to update the screenshots conveniently with docker (they differ per platform)
  • Superclass that simple view models can extend to get a ViewModelSubscriptions for free to reduce boilerplate if they just want to do the simple thing.

FWIW this uses storybook's test-runner for screenshot tests which is considered deprecated in favour of @storybook/addon-vitest which runs vitest in browser mode, but vistest browser mode doesn't actually support screenshots yet! vitest-dev/vitest#6265 is the bug for it. There's a PR, so it might happen soonish, but not yet. Some people have a workaround using jest-image-snapshot but it's ropey. Half an hour before I wrote this message, someone posted a vitest plugin that apparently does it: we could try this.

Checklist

  • Tests written for new code (and old code if feasible).
  • New or updated public/exported symbols have accurate TSDoc documentation.
  • Linter and other CI checks pass.
  • I have licensed the changes to Element by completing the Contributor License Agreement (CLA)

Closes element-hq/wat-internal#397

Turn the trivial TextualEvent into a shared component with a separate view
model for element web. Args to view model will probably change to be more
specific and VM typer needs abstracting out into an interface, but should
give the general idea.
Because we used it anyway, we just cheated by getting it from the context
@dbkr
Copy link
Copy Markdown
Member Author

dbkr commented Jul 9, 2025

  • About storybook's test-runner, do we also using it in compound? Also until element-web-playwright-common is available, non linux user can't update or create screenshots (that will fail in CI)

We don't use this in compound, I'm not sure why not. We have our own script that takes the storybook json file and does broadly the same thing. Maybe test-runner didn't exist when we made compound. The received files are uploaded as an artifact so you can download that and put those in as the screenshots, so it's possible, just annoying.

  • In another PR, we can have the storybook deployed on netlify like in compound (if this is not too much)

Yeah, don't see why we can't do this.

  • In 7cdb91f, I put a wrapper on top of the component in the stories to expose the snapshot properties and the actions in storybook. We can add a kind of generic wrapper to handle most of the use case of the shared components I think. Maybe in another PR too!

Yeah, this seems useful so we can actually use the stories as stories.

@dbkr
Copy link
Copy Markdown
Member Author

dbkr commented Jul 9, 2025

Have added a base class that simple view models can extend in #30297 (and tweaked the subscribe/unsubscribe interface a bit)

as this isn't a hook
Copy link
Copy Markdown
Member

@t3chguy t3chguy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lots of js/ts files with no copyright

Comment thread .github/workflows/shared-component-visual-tests.yaml Outdated
Comment thread .github/workflows/shared-component-visual-tests.yaml
Comment thread jest.config.ts
Comment on lines +21 to +30
this.subs = new ViewModelSubscriptions(this.updateSubscription);
}

private updateSubscription = (): void => {
if (this.subs.listenerCount() > 0) {
this.eventTileProps.mxEvent.on(MatrixEventEvent.SentinelUpdated, this.onEventSentinelUpdated);
} else {
this.eventTileProps.mxEvent.off(MatrixEventEvent.SentinelUpdated, this.onEventSentinelUpdated);
}
};
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this seems a bit foot-gunny, why not a start/stop esque 2-method approach ooi?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I started to think that actually, I changed it in #30297

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ported to here

Comment thread src/viewmodels/event-tiles/TextualEventViewModel.ts Outdated
Comment thread src/viewmodels/ViewModelSubscriptions.ts Outdated
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants