Skip to content
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

fix(toolbar): Remount toolbar rather than block it #1287

Merged
merged 2 commits into from
Jun 4, 2024

Conversation

msfstef
Copy link
Contributor

@msfstef msfstef commented May 21, 2024

Instead of guarding against remounting the toolbar, clean it up by removing the whole shadow dom root and remount it.

Main modification required was to deep clone the style from the template rather than just import it.

Alternative fix to #1280

@thruflo
Copy link
Contributor

thruflo commented May 21, 2024

Thanks for this — if it works fine, I think this is a better approach.

@samwillis
Copy link
Contributor

@msfstef as you mentioned over here: #1280 (comment)

I think this results in the toolbars state being reset if the React component where the toolbar is injected re-renders. If they are injecting from their top level component and edit the file then the state of the toolbar is lost. I don't think that's good DX/UX.

I would continue to argue that that addToolbar should be idempotent, it adds it the first time and is ignored after. This is for use in development, it's not part of a react app.

@msfstef
Copy link
Contributor Author

msfstef commented Jun 3, 2024

@thruflo @samwillis ping to get this either merged or closed, just needs a decision.

I'm happy to make it remount the toolbar, gives it a nice "reset" based idempotency rather than a "check" based one (every addToolbar will clear any existing toolbar and add one, always) at the cost of potentially having a jarring toolbar reset while being used, but I'm not particularly invested in either choice.

@samwillis
Copy link
Contributor

@msfstef lets merge this as it, if users complain about UX then we come back to it.

Copy link
Contributor

@samwillis samwillis left a comment

Choose a reason for hiding this comment

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

👍

@msfstef msfstef merged commit adffce3 into main Jun 4, 2024
10 checks passed
@msfstef msfstef deleted the msfstef/remount-toolbar-rather-than-block branch June 4, 2024 10:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants