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

Store tags state #264

Merged
merged 2 commits into from
Mar 31, 2022
Merged

Store tags state #264

merged 2 commits into from
Mar 31, 2022

Conversation

rtorrero
Copy link
Contributor

tags-store

This PR makes use of the redux store to save the state of the UI components, preventing tags from disappearing when the user changes the view.

It should work for the following views:

  • Hosts
  • Clusters
  • SAP systems*
  • Databases**

Note*: There seems to be some issue with the collapsible that activates when interacting with the tags which prevents the tags from working correctly.

Note**: The tags endpoint for the databases overview one is missing so it can't really be tested yet. Working on that next.

Copy link
Member

@nelsonkopliku nelsonkopliku left a comment

Choose a reason for hiding this comment

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

Nice! Here's some thoughts:

Consider the reducing functions you created

addTagToHost
removeTagFromHost

addTagToCluster
removeTagFromCluster

addTagToDatabase
removeTagFromDatabase

addTagToSAPSystem
removeTagFromSAPSystem

All the addTagToSomething have the same implementation, only differing for the resource being tagged.

All the removeTagFromSomething have the same implementation, only differing for the resource being untagged.

Probably we can find a way to reduce boiler.

Then, these functions have been added to the specific state slices for host, cluster, database, sapSystem, which should be fine.

There's also the option to have a dedicated state slice for tags and merge information with selectors.
Not saying we need to do this other way, just asking if you considered it 😄

Last point: the various addTag and removeTag functions in the components (the ones that actually call the API) might be cleaned up a bit I believe, as they do the exact same thing, with the difference of the resource being handled and the endpoint that gets hit.
This can very well be a followup.

@fabriziosestito
Copy link
Member

There's also the option to have a dedicated state slice for tags and merge information with selectors.
Not saying we need to do this other way, just asking if you considered it smile

Maybe this would be the solution, since i see the duplication here necessary due to the different slices being involved as you pointed out.

Copy link
Contributor

@dottorblaster dottorblaster left a comment

Choose a reason for hiding this comment

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

I like this, we might need a refactoring but we can do that in a follow-up PR. Green light from my side, good job!

@fabriziosestito fabriziosestito merged commit 809d196 into main Mar 31, 2022
@fabriziosestito fabriziosestito deleted the store-tags-state branch March 31, 2022 13:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

4 participants