Skip to content

Conversation

Anemy
Copy link
Member

@Anemy Anemy commented Jan 24, 2022

COMPASS-5277

This PR makes the compass-sidebar package use the favorite connection editing modal from the new connect-form package.
It also adds a focus ring to the button and makes it a button not a for some more accessibility.
This also fixes the issue where, after updating a favorite connection's title we wouldn't update the whole page title (moused over in the demo vid).

These changes take the approach of passing the connectionInfo as a prop to the compass-sidebar component. I'm thinking this might simplify things down the line if many places need connectionInfo as it can be used as a prop and isn't something that we set with the instance model updates. Right now, connectionInfo is passed once on connect with the data-service-connected event, but since at least the favorite of the connection info subject to change, we probably want a cleaner way to update it and notify things that it's updated. I think we could either add the connectionInfo as something available on the instance model, pass it as a prop to components which is what this pr is currently proposing, or maybe something else entirely?

saving.from.sidebar.mp4

Copy link
Collaborator

@gribnoysup gribnoysup left a comment

Choose a reason for hiding this comment

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

if many places need connectionInfo as it can be used as a prop and isn't something that we set with the instance model updates

Looking at the usage of this event, only sidebar is using the connectionInfo right now, so I don't think it's something we should be concerned about. More than that, the way we pass whole models from the redux stores to the react components is a big issue in Compass right now that causes unnecessary rerenders almost everywhere in the application affecting app performance. Passing whole models around as props just replicates the same behavior without using redux.

It also feels pretty cumbersome that we have to manually pass this state and a modal component, and wire everything up to make the editing work. What if we need to add this modal to another place in the app? We would need to repeat this whole thing again, potentially degrading rendering performance for parts of the app that don't even care about this state for the reasons mentioned above. What if component interface changes? We would need to introduce changes in multiple places in the app even though nothing really changes, which hints to me that this leaks too much implementation details to the places of the app that shouldn't be aware of it.

I said this a couple of times already and I still believe that it's not a good idea to introduce new approaches on top of existing ones without a strategy, I get that we already did it for compass-home, but I would at least try to avoid mixing approaches in plugins that already exist.

It's ultimately your call if you want to change anything here, I don't want to block this work as it seems like we are trying to wrap this up as soon as possible, but I genuinely don't think it's a good idea that the whole UI tree needs to be aware of the connectionInfo state (or any other truly global state, like instance model one for example) passed down manually from the very root of the application.

Copy link
Collaborator

@gribnoysup gribnoysup left a comment

Choose a reason for hiding this comment

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

Thanks for changing sidebar plugin back to using redux for global state. When we inevitably will need to clean up how this plugin passes state to UI later down the road, I'm sure it will be much easier to figure our where global state is coming from as this will only be one place.

Overall looks good to me, left a few small comments for things that caught my eye

@Anemy Anemy merged commit 1d97624 into main Jan 27, 2022
@Anemy Anemy deleted the COMPASS-5277-use-new-favorite-connection-modal-in-sidebar branch January 27, 2022 17:16
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