matrix-react-sdk icon indicating copy to clipboard operation
matrix-react-sdk copied to clipboard

Make widget theme reactive - Update widget url when the theme changes

Open toger5 opened this issue 1 year ago • 1 comments

Signed-off-by: Timo K [email protected]

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.
  • [ ] Sign-off given on the changes (see CONTRIBUTING.md).

toger5 avatar Feb 28 '24 19:02 toger5

depends on where in the url the widget is adding the theme. If it is adding it to the hash it will not reload the page.

This needs to be documented so that widgets that cannot just be reloaded are aware, that the theme part of the url can change on the fly.

What do you think about only updating it if the url has the theme in the hash? Another idea to improove that situation would be to make this a widget capability. So the default would be to not change the url onChange. Widget devs have to opt in to make url change on theme change and they can only do so if that is compatible with their widget like with EC.

The last solution could be to make this a EC specific feature. So it will not happen for any other widget except EC.

toger5 avatar Feb 28 '24 20:02 toger5

How about sending the theme update over postMessage?

t3chguy avatar Feb 29 '24 09:02 t3chguy

How about sending the theme update over postMessage?

That was the initial plan but this is simpler and does not require a new msc. (action would need changes in: react-sdk, rust-sdk, matrix-widget-api, element-call + new spec)

I still think the action would be the most conventional solution. It just would be a much bigger work piece. Maybe worthit though. I like this workaround to support the light theme soonish. It reuses the url template system and the theme property that already exists there.

I think my favorite solution would be to special case the url update to EC and give it a big comment, that this has to be done with a new widget action in the future. WDYT?

toger5 avatar Feb 29 '24 11:02 toger5

That was the initial plan but this is simpler and does not require a new msc.

It wouldn't - given it could be io.element.theme

I don't think a solution where only the hash changes is plausible either given the SPA may clear whatever context the user is in on a route change, e.g. lose text that has been input, unless it was locked behind a capability

An EC specific thing is probably fine but kinda ugly

t3chguy avatar Feb 29 '24 12:02 t3chguy

It wouldn't - given it could be io.element.theme

Right. I would prefer doing it properly however. But that is another stop gap like solution that already paves the path for actually making this a widget api feature.

Since it was super low effort I updated it to be EC specific for now. + Added a comment that this is a stop gap. It is an edge case anyways to change the theme while in a call.

Maybe next week I can put some more time into this and refactor it to use a widget action but I cannot promise that. (I would like to see it working as soon as possible but I fully understand if we dont want to merge it like this as a stop gap.)

toger5 avatar Mar 01 '24 13:03 toger5