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

[Infra UI] Selectively persist UI state in the url (via container components) #22980

Conversation

weltenwort
Copy link
Member

@weltenwort weltenwort commented Sep 12, 2018

This PR implements syncing of local redux state to the URL via container components. It thereby supersedes #22742, which was the first attempt to achieve that using epics.

UrlStateContainer

The new UrlStateContainer component can be used to synchronize a piece of state with a given search parameter urlStateKey in the URL. It treats the value of that search parameter like a controlled input, which means it persists the state given via the urlState property rison-encoded to the URL while preserving all other search parameters. Any external changes to the value in the managed search parameter are processed using the function given via the mapToUrlState property the return value of which is passed (along with the previous value) to the function supplied in the onChange property. In the onChange function any number of appropriate redux actions (or whatever the state management uses for mutations) can then be dispatched. On initial mounting of the component, the onInitialize property is called (if supplied) to allow for state initialization at page load or route navigation time.

The controlled nature of the search parameter value means that any incoming change that does not lead to a corresponding change in the state will be discarded, because the value is completely derived from the urlState.

URL State Containers for the store slices

Each of the local waffle_time, waffle_filter, log_position, log_filter, log_minimap and log_textview slices have parts of their state persisted to and restored from the URL via containers rendered on their respective pages. Within this PR, the persisted pieces of state are:

  • waffle map time
  • waffle map auto-reload state
  • waffle map filter
  • log scroll position
  • log live-stream state
  • log filter
  • log minimap scale
  • log text size and wrap

@weltenwort weltenwort added WIP Work in progress :Ingest UI Feature:Metrics UI Metrics UI feature labels Sep 12, 2018
@weltenwort weltenwort self-assigned this Sep 12, 2018
@weltenwort weltenwort added the loe:medium Medium Level of Effort label Sep 12, 2018
@weltenwort weltenwort force-pushed the infra-ui-enhancement-add-url-sync-container branch from a909e30 to 272bb73 Compare September 12, 2018 23:26
@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@weltenwort weltenwort changed the title [Infra UI] [WIP] Selectively persist UI state in the url (via container components) [Infra UI] Selectively persist UI state in the url (via container components) Sep 13, 2018
@weltenwort weltenwort added review and removed WIP Work in progress labels Sep 13, 2018
export const WithLogFilterUrlState = () => (
<WithLogFilter>
{({ applyFilterQuery, filterQuery }) => (
<UrlStateContainer<LogFilterUrlState>
Copy link
Member

Choose a reason for hiding this comment

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

The mixing TS generics and JSX together reads weird. Can you assign that to a variable then just use the variable? I haven't tried that before so I'm not sure if it will work but it if it does it might make it more readable.

Copy link
Member Author

@weltenwort weltenwort Sep 13, 2018

Choose a reason for hiding this comment

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

Unfortunately that is not easily possible. The generic type argument has to be passed at instantiation time, which is handled by JSX. If I would convert the UrlStateContainer component into a class-based component, a no-op inheritance could nail down the type. Alternatively, we could manually type the passed properties sufficiently so the inference figures it out. Typing the return type of mapToUrlState should do it. Would you think that improves the situation?

<UrlStateContainer
  urlState={urlState}
  urlStateKey="logPosition"
  mapToUrlState={mapToUrlState}
  ...
/>

const mapToUrlState = (value: any): LogPositionUrlState | undefined => ...

Copy link
Member

Choose a reason for hiding this comment

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

Yeah... that's a lot easier to read.

Copy link
Member Author

Choose a reason for hiding this comment

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

ok, change coming up...

As per review feedback the container type annotations are not explicitly
stated in JSX, but instead inferred through the return type of the
`mapToUrlState` function.
@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@weltenwort
Copy link
Member Author

@simianhacker ok, I worked around the type arguments in JSX 👆

Copy link
Contributor

@skh skh left a comment

Choose a reason for hiding this comment

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

Functionality looks good 👍

@weltenwort
Copy link
Member Author

@simianhacker are you satisfied with the changes I made to the PR?

@simianhacker simianhacker mentioned this pull request Sep 18, 2018
4 tasks
@simianhacker
Copy link
Member

LGTM

@weltenwort weltenwort merged commit d9f423d into elastic:feature-infra-ui Sep 18, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature:Metrics UI Metrics UI feature loe:medium Medium Level of Effort review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants