[Index management] Add support in url for hidden toggle#66715
Conversation
Tests probably have broken
Also remove unused translations
This allows setting the value of the "inlcude hidden" toggle via the url hash query param "includeHidden". Also added a test.
|
Pinging @elastic/es-ui (Team:Elasticsearch UI) |
…ent/add-support-in-url-for-hidden-toggle * 'master' of github.com:elastic/kibana: (34 commits) [SIEM][CASE] Fix bug when connector is deleted. (elastic#65876) [SIEM][CASE] Improve layout (elastic#66232) [Index Management] Support Hidden Indices (elastic#66422) Add Login Selector functional tests. (elastic#65705) Lens drilldowns (elastic#65675) [ML] Custom template for apiDoc markdown (elastic#66567) Don't bootstrap core type emits (elastic#66377) [Dashboard] Improve loading error handling (elastic#66372) [APM] Minor style fixes for the node strokes (elastic#66574) [Ingest Manager] Fix create data source from integration (elastic#66626) [Metrics UI] Fix default metric alert interval for new conditions (elastic#66610) [Metrics UI] Fix alignment and allow clearing metric value (elastic#66589) Don't return package name for non-package data streams (elastic#66606) [Ingest Manager] Consolidate routing and add breadcrumbs to all pages (elastic#66475) [Docs/Reporting] Have the docs about granular timeout match Cloud docs (elastic#66267) Don't automatically add license header to code inside plugins dir. (elastic#66601) [APM] Don't trigger map layout if no elements (elastic#66625) [Logs UI] Validate ML job setup time ranges (elastic#66426) Fix pagination bugs in CCR and Remote Clusters (elastic#65931) Add cloud icon for supported settings and embed single-sourced getting started (elastic#65610) ... # Conflicts: # x-pack/plugins/index_management/public/application/sections/home/index_list/index_table/index_table.js # x-pack/plugins/index_management/server/lib/fetch_indices.ts
There was a problem hiding this comment.
Tested locally, code looks good! I do think we need to tweak the data flow so that changing the toggle value updates the URL, which then causes the component to pull the new value from the URL and update the state.
The reason I think this is because eventually we'll want to add support for deep-linking to an index so people can pass around a URL to each other. If this index is hidden, then a user will click the toggle to show hidden indices before clicking a hidden index to view its details and then sharing the URL. The URL in this case will need to contain the hidden toggle state so that the hidden index is revealed when the URL is visited. WDYT?
x-pack/plugins/index_management/__jest__/client_integration/helpers/home.helpers.ts
Outdated
Show resolved
Hide resolved
|
I agree with @cjceniza, the URL should be the source of truth where our internal states get its value. 👍 |
|
@elasticmachine merge upstream |
|
ignoring request to update branch, pull request is closed |
|
@elasticmachine merge upstream |
The component toggle gets it's initial state form hash query param and thereafter the toggle drives the state in the url.
Thanks for the review @cjcenizal and @sebelga ! That makes total sense, I was only considering the case where we go Data Streams -> Index Template (not generating and sharing URL from Index Template). I've updated the flow to be:
Would you mind taking another look-see? |
cjcenizal
left a comment
There was a problem hiding this comment.
Looks good! Feel free to merge, though I think we can take this a step further (see comment).
| } | ||
|
|
||
| // Check if the we have the includeHidden query param | ||
| const { includeHidden } = parse((location && location.search) || ''); |
There was a problem hiding this comment.
I was thinking that we could remove showHiddenIndices from the store entirely and derive it from the URL inside of the render method. Then we don't need to worry about keeping the two in sync -- the URL is the source of truth for this state, as Seb mentioned.
render() {
// snip
const { includeHidden } = parse((location && location.search) || '');
const showHiddenIndices = includeHidden === 'true';There was a problem hiding this comment.
I think I can see how that would work. We'd still need to hold a piece of local state to trigger a re-render when the toggle gets clicked however: click toggle -> update url -> this.setState -> render. To make it fully reliant on hash we can window.addEventListener('hashchange', () => { this.setState })- I am not convinced we should expect users to drive state this way though.
We still need to sync the URL on toggle, the difference being where (derived from store or derived from local state).
I think the current implementation is in keeping with the established pattern in this plugin so I would prefer to stick with it, but I can see how we can change this up when building bigger improvements in future.
There was a problem hiding this comment.
I think the current implementation is in keeping with the established pattern in this plugin so I would prefer to stick with it
That sounds good to me. Also, I looked a little more deeply and it looks like there's a getFilteredIndices selector that depends upon tableState.showHiddenIndices, so removing it would be more involved than I thought (and possibly make the code more difficult to follow).
There was a problem hiding this comment.
If the URL is the source of truth, this is what we want to do
import React, { useEffect } from 'react';
import { useLocation } from 'react-router';
function MyComponent() {
const location = useLocation();
useEffect(() => {
console.log('route has been changed');
...your code
},[location.pathname]);
}Then the toggle is only responsible to update the URL.
There was a problem hiding this comment.
Ah, that is cool, I did not know about useLocation. Is there an equivalent approach for class components?
There was a problem hiding this comment.
This should do the same thing
import {withRouter} from 'react-router-dom';
class MyComponent extends PureComponent {
componentDidUpdate(prevProps) {
if (this.props.location.pathname !== prevProps.location.pathname) {
console.log('route has been changed');
...your code
}
}
render() {
return (
...
);
}
}
export default withRouter(MyComponent);…ent/add-support-in-url-for-hidden-toggle * 'master' of github.com:elastic/kibana: (49 commits) [Uptime] Improve responsiveness details page (elastic#67034) skip flaky suite (elastic#66669) Revert "Integration of a static filesystem for the node_modules (elastic#47998)" (elastic#67124) Support api_integration/kibana/stats against remote hosts (elastic#53000) chore(NA): add module name mapper for src plugins on x-pack (elastic#67103) Change the error message on TSVB in order to be more user friendly (elastic#67090) [kbn/optimizer] poll parent process to avoid zombie processes (elastic#67059) [Visualize] Lazy load default editor, fix duplicated styles (elastic#66732) Bump styled-component dependencies (elastic#66611) Bump react-markdown dependencies (elastic#66615) Fix Core docs links (elastic#66977) Timelion graph is not refreshing content after searching or filtering (elastic#67023) Remove `--xpack.endpoint.enabled=true` from README.md file (elastic#67053) Move apm tutorial from apm plugin into apm_oss plugin (elastic#66432) [Logs UI] Restore call to `UsageCollector.countLogs` (elastic#67051) Remove unused license check result from LP Security plugin (elastic#66966) [Saved Objects] adds support for including hidden types in saved objects client (elastic#66879) [Discover] Deangularize timechart header (elastic#66532) [Discover] Improve and unskip a11y context view test (elastic#66959) [SIEM] Refactor Timeline.timelineType draft to Timeline.status draft (elastic#66864) ... # Conflicts: # x-pack/plugins/index_management/__jest__/client_integration/helpers/home.helpers.ts
|
@elasticmachine merge upstream |
💚 Build SucceededHistory
To update your PR or re-run it, just comment with: |
* First iteration of supporting hidden indices in indices table Tests probably have broken * Remove unused code * Fix logic when calling get index endpoint with empty array Also remove unused translations * First iteration of new flag being passed in through URL * Added includeHidden param to index management list view This allows setting the value of the "inlcude hidden" toggle via the url hash query param "includeHidden". Also added a test. * Remove unused variable * Revise data flow The component toggle gets it's initial state form hash query param and thereafter the toggle drives the state in the url. * Fix typo * Delete ts-ignore * Slight update to test and comment placement for clarity * Fix copy-pasta Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
* master: Fix Canvas nav link in env with spaces (elastic#67127) Fix Canvas fullscreen with new fixed header (elastic#67131) [Index management] Add support in url for hidden toggle (elastic#66715)
) * First iteration of supporting hidden indices in indices table Tests probably have broken * Remove unused code * Fix logic when calling get index endpoint with empty array Also remove unused translations * First iteration of new flag being passed in through URL * Added includeHidden param to index management list view This allows setting the value of the "inlcude hidden" toggle via the url hash query param "includeHidden". Also added a test. * Remove unused variable * Revise data flow The component toggle gets it's initial state form hash query param and thereafter the toggle drives the state in the url. * Fix typo * Delete ts-ignore * Slight update to test and comment placement for clarity * Fix copy-pasta Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com> Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
Summary
Gives the index list view in Index Management the ability to read a query param from the URL which will toggle the "include hidden indices" toggle above the table.
This work is preparatory for the linking functionality that will be added with Data streams.
How to test
Currently there is no why in the UI to toggle this so it can be tested by:
?includeHidden=trueand press enterChecklist