Skip to content
This repository has been archived by the owner on Sep 11, 2024. It is now read-only.

Make EC widget theme reactive - Update widget url when the theme changes #12295

Merged
merged 17 commits into from
Mar 13, 2024

Conversation

toger5
Copy link
Contributor

@toger5 toger5 commented Feb 28, 2024

Signed-off-by: Timo K toger5@hotmail.de

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).

Signed-off-by: Timo K <toger5@hotmail.de>
Copy link
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.

This would cause you to be kicked out of conferences, docs, games etc, we should not be causing the widget to reload without user intervention, they may lose whatever they were typing etc.

@toger5
Copy link
Contributor Author

toger5 commented Feb 28, 2024

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.

@t3chguy
Copy link
Member

t3chguy commented Feb 29, 2024

How about sending the theme update over postMessage?

@toger5
Copy link
Contributor Author

toger5 commented Feb 29, 2024

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?

@t3chguy
Copy link
Member

t3chguy commented Feb 29, 2024

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

@toger5 toger5 added the T-Enhancement New features, changes in functionality, performance boosts, user-facing improvements label Feb 29, 2024
@toger5
Copy link
Contributor Author

toger5 commented Mar 1, 2024

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.)

Copy link
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.

Looks like it needs test coverage to be merged

src/components/views/elements/AppTile.tsx Outdated Show resolved Hide resolved
Signed-off-by: Timo K <toger5@hotmail.de>
Signed-off-by: Timo K <toger5@hotmail.de>
Signed-off-by: Timo K <toger5@hotmail.de>
Signed-off-by: Timo K <toger5@hotmail.de>
Signed-off-by: Timo K <toger5@hotmail.de>
Signed-off-by: Timo K <toger5@hotmail.de>
src/theme.ts Outdated Show resolved Hide resolved
src/settings/watchers/ThemeWatcher.ts Outdated Show resolved Hide resolved
Signed-off-by: Timo K <toger5@hotmail.de>
Signed-off-by: Timo K <toger5@hotmail.de>
Signed-off-by: Timo K <toger5@hotmail.de>
@toger5 toger5 added this pull request to the merge queue Mar 13, 2024
@toger5 toger5 changed the title Make widget theme reactive - Update widget url when the theme changes Make EC widget theme reactive - Update widget url when the theme changes Mar 13, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Mar 13, 2024
@toger5 toger5 added this pull request to the merge queue Mar 13, 2024
Merged via the queue into develop with commit c42562e Mar 13, 2024
26 checks passed
@toger5 toger5 deleted the toger5/reactive-widget-theme-url branch March 13, 2024 15:15
thoraj added a commit to verji/matrix-react-sdk that referenced this pull request Mar 14, 2024
* Upgrade dependency to matrix-js-sdk@31.5.0-rc.0

* v3.94.0-rc.0

* Handle up/down as well as left/right for horizontal toolbars for improved a11y (matrix-org#12305)

Signed-off-by: Michael Telatynski <7t3chguy@gmail.com>

* Remove references to internal js-sdk type `CryptoBackend` (matrix-org#12321)

* Remove references to internal js-sdk type `CryptoBackend`

* Use `Paramteters` to avoid `ts-ignore`

* Use `strong` element to semantically denote visually emphasised content (matrix-org#12320)

* Use `strong` element to semantically denote visually emphasised content

Signed-off-by: Michael Telatynski <7t3chguy@gmail.com>

* Update snapshots

Signed-off-by: Michael Telatynski <7t3chguy@gmail.com>

* Add comment

Signed-off-by: Michael Telatynski <7t3chguy@gmail.com>

---------

Signed-off-by: Michael Telatynski <7t3chguy@gmail.com>

* Remove unused slider component (matrix-org#12303)

It is unused as of matrix-org#12246. I noticed this while working on matrix-org#12299.

* Update matrix-org (matrix-org#11966)

Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>

* Update all non-major dependencies (matrix-org#12322)

Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>

* Update dependency stylelint-scss to v6.2.0 (matrix-org#12323)

Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>

* Update dependency @vector-im/compound-web to v3.1.3 (matrix-org#12281)

* Update dependency @vector-im/compound-web to v3.1.3

* Update snapshots

Signed-off-by: Michael Telatynski <7t3chguy@gmail.com>

* Update snapshots

Signed-off-by: Michael Telatynski <7t3chguy@gmail.com>

* Update snapshots

Signed-off-by: Michael Telatynski <7t3chguy@gmail.com>

* Fix TAC width due to compound update (matrix-org#12326)

---------

Signed-off-by: Michael Telatynski <7t3chguy@gmail.com>
Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Co-authored-by: Michael Telatynski <7t3chguy@gmail.com>
Co-authored-by: Florian Duros <florianduros@element.io>

* Call guest access link creation to join calls as a non registered user via the EC SPA (matrix-org#12259)

* Add externall call link button if in public call room

Signed-off-by: Timo K <toger5@hotmail.de>

* Allow configuring a spa homeserver url.

Signed-off-by: Timo K <toger5@hotmail.de>

* temp

Signed-off-by: Timo K <toger5@hotmail.de>

* remove homeserver url

Signed-off-by: Timo K <toger5@hotmail.de>

* Add custom title to share dialog.
So that we can use it as a "share call" dialog.

Signed-off-by: Timo K <toger5@hotmail.de>

* - rename config options
- only show link button if a guest url is provided
- share dialog custom Title
- rename call share labels

Signed-off-by: Timo K <toger5@hotmail.de>

* rename to title_link

Signed-off-by: Timo K <toger5@hotmail.de>

* add tests for ShareDialog

Signed-off-by: Timo K <toger5@hotmail.de>

* add tests for share call button

Signed-off-by: Timo K <toger5@hotmail.de>

* review

Signed-off-by: Timo K <toger5@hotmail.de>

* remove comment

Signed-off-by: Timo K <toger5@hotmail.de>

* Update src/components/views/dialogs/ShareDialog.tsx

Co-authored-by: David Baker <dbkr@users.noreply.github.com>

---------

Signed-off-by: Timo K <toger5@hotmail.de>
Co-authored-by: David Baker <dbkr@users.noreply.github.com>

* Fix spotlight opening in TAC (matrix-org#12315)

* Fix spotlight opening in TAC

* Add tests

* Remove `RovingTabIndexProvider`

* Reset power selector on API failure to prevent state mismatch (matrix-org#12319)

* Reset power selector on API failure to prevent state mismatch

Signed-off-by: Michael Telatynski <7t3chguy@gmail.com>

* Allow onChange to be sync or async

Signed-off-by: Michael Telatynski <7t3chguy@gmail.com>

* Add unmounted check

Signed-off-by: Michael Telatynski <7t3chguy@gmail.com>

* Improve coverage

Signed-off-by: Michael Telatynski <7t3chguy@gmail.com>

* Iterate

Signed-off-by: Michael Telatynski <7t3chguy@gmail.com>

---------

Signed-off-by: Michael Telatynski <7t3chguy@gmail.com>

* Use correct push rule to evaluate room-wide mentions (matrix-org#12318)

Signed-off-by: Michael Telatynski <7t3chguy@gmail.com>

* Improve Forward Dialog a11y by switching to roving tab index interactions (matrix-org#12306)

* Improve Forward Dialog a11y by switching to roving tab index interactions

Signed-off-by: Michael Telatynski <7t3chguy@gmail.com>

* Improve screen reader readout

Signed-off-by: Michael Telatynski <7t3chguy@gmail.com>

* Improve screen reader readout

Signed-off-by: Michael Telatynski <7t3chguy@gmail.com>

* Add tests

Signed-off-by: Michael Telatynski <7t3chguy@gmail.com>

---------

Signed-off-by: Michael Telatynski <7t3chguy@gmail.com>

* [create-pull-request] automated change (matrix-org#12330)

Co-authored-by: github-merge-queue <github-merge-queue@users.noreply.github.com>

* Reuse media content/info types from the js-sdk (matrix-org#12308)

* TAC: Fix CSS & component typos (matrix-org#12333)

* Fix CSS & component typo

* Update snapshots

* Element Call: fix widget shown while its still loading (`waitForIframeLoad=false`) (matrix-org#12292)

* show loading spinner also if waitForIframeLoad = false
Configure EC so it waits for the content loaded action
!WARNING This breaks compatibility with the full mesh branch.
I would like to discuss here if/when we can do that.

Signed-off-by: Timo K <toger5@hotmail.de>

* stop show loading screen on widget ready (instead of preparing)

Signed-off-by: Timo K <toger5@hotmail.de>

* wait until widget loading is over before comparing screenshots

Signed-off-by: Timo K <toger5@hotmail.de>

* fix waitForIFrame=true widgets

Signed-off-by: Timo K <toger5@hotmail.de>

* test

Signed-off-by: Timo K <toger5@hotmail.de>

* always start with loading true. + cleanup

Signed-off-by: Timo K <toger5@hotmail.de>

* simplify loading

Signed-off-by: Timo K <toger5@hotmail.de>

* update snapshots (start not in loading state for waitForIframe = true widgets)

Signed-off-by: Timo K <toger5@hotmail.de>

---------

Signed-off-by: Timo K <toger5@hotmail.de>

* Upgrade dependency to matrix-js-sdk@31.5.0

* v3.94.0

* Resetting package fields for development

* Reset matrix-js-sdk back to develop branch

* Refine styles of menus, toasts, popovers, and modals (matrix-org#12332)

* Refine styles of menus, toasts, popovers, and modals

This is a reintroduction of matrix-org#12247, with the difference that modal styles have now been refreshed as well.

* Restore the fixed heights of some dialogs

* Fix formatting and flaky screenshot

* Make EC widget theme reactive - Update widget url when the theme changes (matrix-org#12295)

* update widget url when the theme changes

Signed-off-by: Timo K <toger5@hotmail.de>

* quick "make it EC specific" workaround proposal.

Signed-off-by: Timo K <toger5@hotmail.de>

* use `matches`

Signed-off-by: Timo K <toger5@hotmail.de>

* test coverage

Signed-off-by: Timo K <toger5@hotmail.de>

* more test coverage

Signed-off-by: Timo K <toger5@hotmail.de>

* fix jest

Signed-off-by: Timo K <toger5@hotmail.de>

* add tests for theme changes

Signed-off-by: Timo K <toger5@hotmail.de>

* update snapshots

Signed-off-by: Timo K <toger5@hotmail.de>

* test for theme update with non ec widget

Signed-off-by: Timo K <toger5@hotmail.de>

* add dark custom theme widget url

Signed-off-by: Timo K <toger5@hotmail.de>

* trigger conditions for theme cleanup

Signed-off-by: Timo K <toger5@hotmail.de>

* update tests using testId

Signed-off-by: Timo K <toger5@hotmail.de>

* use typed event emitter for theme watcher

Signed-off-by: Timo K <toger5@hotmail.de>

* simplify condition

Signed-off-by: Timo K <toger5@hotmail.de>

---------

Signed-off-by: Timo K <toger5@hotmail.de>

---------

Signed-off-by: Michael Telatynski <7t3chguy@gmail.com>
Signed-off-by: Timo K <toger5@hotmail.de>
Co-authored-by: RiotRobot <releases@riot.im>
Co-authored-by: Michael Telatynski <7t3chguy@gmail.com>
Co-authored-by: Richard van der Hoff <1389908+richvdh@users.noreply.github.com>
Co-authored-by: Robin <robin@robin.town>
Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Co-authored-by: Florian Duros <florianduros@element.io>
Co-authored-by: Timo <16718859+toger5@users.noreply.github.com>
Co-authored-by: David Baker <dbkr@users.noreply.github.com>
Co-authored-by: github-merge-queue <github-merge-queue@users.noreply.github.com>
thoraj added a commit to verji/matrix-react-sdk that referenced this pull request Mar 14, 2024
andybalaam pushed a commit that referenced this pull request Mar 18, 2024
…ges (#12295)

* update widget url when the theme changes

Signed-off-by: Timo K <toger5@hotmail.de>

* quick "make it EC specific" workaround proposal.

Signed-off-by: Timo K <toger5@hotmail.de>

* use `matches`

Signed-off-by: Timo K <toger5@hotmail.de>

* test coverage

Signed-off-by: Timo K <toger5@hotmail.de>

* more test coverage

Signed-off-by: Timo K <toger5@hotmail.de>

* fix jest

Signed-off-by: Timo K <toger5@hotmail.de>

* add tests for theme changes

Signed-off-by: Timo K <toger5@hotmail.de>

* update snapshots

Signed-off-by: Timo K <toger5@hotmail.de>

* test for theme update with non ec widget

Signed-off-by: Timo K <toger5@hotmail.de>

* add dark custom theme widget url

Signed-off-by: Timo K <toger5@hotmail.de>

* trigger conditions for theme cleanup

Signed-off-by: Timo K <toger5@hotmail.de>

* update tests using testId

Signed-off-by: Timo K <toger5@hotmail.de>

* use typed event emitter for theme watcher

Signed-off-by: Timo K <toger5@hotmail.de>

* simplify condition

Signed-off-by: Timo K <toger5@hotmail.de>

---------

Signed-off-by: Timo K <toger5@hotmail.de>
Half-Shot added a commit that referenced this pull request Mar 27, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
T-Enhancement New features, changes in functionality, performance boosts, user-facing improvements
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants