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

Make the UI Editor work through portals #53

Merged
merged 2 commits into from
Jan 15, 2021
Merged

Conversation

brandones
Copy link
Collaborator

The core feature here is to get all the UI Editor code into implementer-tools. Changing the UI Editor shouldn't require changing the Extension or ExtensionSlot elements themselves. This change achieves that by using Portals to get the UI Editor content to appear over the slots and extensions.

In order to accomplish this, the main thing that had to change in Core is that 1. slots need to pass a ref to register with and 2. DOM nodes for slots and extensions should be kept in the store.

Tangentially, I created a function createUseStore based on this comment from the unistore creator to make it easier to use state and actions in React components (no more connect).

There's pretty much no UI change here. Just an architecture thing. We can now make the UI Editor as elaborate as we want without worrying about the core libraries getting bloated or slowing down.

Screenshot from 2020-12-24 14-10-33

Copy link
Contributor

@FlorianRappl FlorianRappl left a comment

Choose a reason for hiding this comment

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

LGTM; I am not sure (actually I would not advise this) to have the domElement in the global state container. Here, we must be sure that it does always get properly cleaned up - otherwise, we'll end up in a bad spot. I just assume it works fine right now so let's just leave it.

Have you checked if the rendering is efficient? I checked beforehand / when implementing and extensions had the least amount of renders (i.e., 1). Is this still the case or does the update with the respective DOM element change this?

@brandones
Copy link
Collaborator Author

I am not sure (actually I would not advise this) to have the domElement in the global state container. Here, we must be sure that it does always get properly cleaned up - otherwise, we'll end up in a bad spot.

Good point, we will need to be very careful about that. If you have another suggestion it would be welcome, but this is the only way I thought of.

Have you checked if the rendering is efficient?

I hadn't! I instrumented the "Active Visits" button and the home page that contains it and these were the results:

Before this change:
Screenshot from 2021-01-15 10-01-05

After this change:
Screenshot from 2021-01-15 10-06-56

So it appears that (in either case) there is some nondeterminism having to do with the order in which things are loaded, which determines whether there's one render or multiple. Prior to this change, there would be two renders in the worse case. With this change, there are three.

I'll investigate whether it's practical to either 1. ensure that the more efficient ordering happens deterministically or 2. eliminate the extra render that this PR introduces for the worse case.

@brandones brandones merged commit 879042c into master Jan 15, 2021
@brandones brandones deleted the ui-editor-portal branch January 15, 2021 18:13
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.

2 participants