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

Remove default values from URL state hash #3030

Merged
merged 2 commits into from
Jan 22, 2018

Conversation

fbarl
Copy link
Contributor

@fbarl fbarl commented Jan 18, 2018

Resolves #1727.

Changes

major

  • Use only non-default values for topologyOptions in Scope state. These are calculated via new helper hashDifferenceDeep which basically omits all the deep non-hash values which correspond literally to default options.
  • Filter out all (other) URL state fields which correspond to their initial Redux state.

minor

  • Removed label field from nodeDetails Scope state hash array, as it wasn't used anywhere.
  • Changed searchQuery Redux state default to '' for better matching with the URL state.

Note: I'm aware that topologyId field could also have been omitted in the URL in case it has a default value, but that default topology is somewhat arbitrary and I didn't want to make a special case of it.

@fbarl fbarl self-assigned this Jan 18, 2018
@fbarl fbarl force-pushed the 1727-remove-default-values-from-url-hash branch from 74093d4 to 0de0a9a Compare January 19, 2018 15:35
@fbarl fbarl changed the title [WIP] Remove default values from URL state hash Remove default values from URL state hash Jan 19, 2018
@fbarl fbarl requested a review from foot January 19, 2018 15:43
Copy link
Contributor

@foot foot left a comment

Choose a reason for hiding this comment

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

Nice! LGTM, if feasible would be cool to include topologyId too, which "defaults" to kube-controllers as its rank is 0.

@@ -661,7 +658,7 @@ export function rootReducer(state = initialState, action) {
state = setTopology(state, state.get('currentTopologyId'));
// only set on first load, if options are not already set via route
if (!state.get('topologiesLoaded') && state.get('topologyOptions').size === 0) {
state = setDefaultTopologyOptions(state, state.get('topologies'));
state = state.set('topologyOptions', getDefaultTopologyOptions(state));

This comment was marked as abuse.

@fbarl
Copy link
Contributor Author

fbarl commented Jan 22, 2018

if feasible would be cool to include topologyId too, which "defaults" to kube-controllers as its rank is 0.

This is not an ideal argument against it, but that change would make it possible to have an empty state hash (if we're on all defaults), which would be tricky to distinguish from no state at all which would default to localStorage. Something to consider in a future PR perhaps, but I think we can make a small exception with topologyId field for now and always display it.

@fbarl fbarl force-pushed the 1727-remove-default-values-from-url-hash branch from 7316b75 to 0176536 Compare January 22, 2018 14:44
@fbarl fbarl force-pushed the 1727-remove-default-values-from-url-hash branch from 0176536 to 7e729dd Compare January 22, 2018 14:53
@fbarl fbarl merged commit 66b9185 into master Jan 22, 2018
@fbarl fbarl deleted the 1727-remove-default-values-from-url-hash branch February 9, 2018 08:25
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