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

Extending stateful URLS with node filters and Expanding/collapsing modular pipelines flag #1799

Merged
merged 48 commits into from
Apr 12, 2024
Merged
Show file tree
Hide file tree
Changes from 46 commits
Commits
Show all changes
48 commits
Select commit Hold shift + click to select a range
1bdd7ae
shareable viz with multiple platform UI
jitu5 Jan 24, 2024
bfb8a66
test fix
jitu5 Jan 25, 2024
ecbcdbb
Relative path fix for user entered url
jitu5 Jan 25, 2024
8b49f52
Test updated
jitu5 Jan 26, 2024
11768e5
Test updated
jitu5 Jan 26, 2024
a2460b6
refactor router to accept new deployer inputs (#1739)
ravi-kumar-pilla Feb 8, 2024
9c86282
Refactor Shareableviz CLI (#1740)
ravi-kumar-pilla Feb 8, 2024
25a7aff
Kedro Viz Static Website hosting on Azure (#1708)
ravi-kumar-pilla Feb 13, 2024
8f5a152
Kedro Viz Static Website Hosting on GCP (#1711)
ravi-kumar-pilla Feb 13, 2024
17e100a
Initial commit
jitu5 Feb 15, 2024
a726b70
Set query params from local storage on load
jitu5 Feb 16, 2024
ff643a5
Merge branch 'feature/share_viz_ui' into feature/stateful_url
jitu5 Feb 16, 2024
ff55085
Merge branch 'feature/shareableviz-extended-support' into feature/sta…
jitu5 Feb 16, 2024
2106bf3
Merge branch 'main' into feature/stateful_url
jitu5 Feb 26, 2024
d763d11
Test fix
jitu5 Feb 26, 2024
fefbbdb
test fix
jitu5 Feb 26, 2024
3d37c13
Test fixes
jitu5 Feb 29, 2024
5c380f6
Merge branch 'main' into feature/stateful_url
jitu5 Feb 29, 2024
c69f4c2
Some method docs added.
jitu5 Mar 11, 2024
7dfe800
Shortening existing URL query params focused_id, selected_id, selecte…
jitu5 Mar 12, 2024
db7b4dd
Fix experiment tracking search query test
jitu5 Mar 12, 2024
0c7b267
Merge branch 'main' into feature/stateful_url
jitu5 Mar 12, 2024
248ea6d
Clear filter button added
jitu5 Mar 12, 2024
03c6b42
Missing test added
jitu5 Mar 13, 2024
b80730b
Release note added
jitu5 Mar 15, 2024
fdc13cb
Clear to Reset
jitu5 Mar 20, 2024
4397923
Release note & retain qparams updated
jitu5 Mar 20, 2024
a300bed
fixes
rashidakanchwala Mar 26, 2024
719fb31
almost done
rashidakanchwala Mar 26, 2024
d176e7b
missing nodeTypes added
jitu5 Apr 2, 2024
187b338
URL Params handling moved to user-generated-pathname.js
jitu5 Apr 2, 2024
d90c158
Review suggestion added
jitu5 Apr 2, 2024
5b65eb4
getKeysByValue moved
jitu5 Apr 2, 2024
0a85e0e
updateStateWithFilters renamed
jitu5 Apr 2, 2024
5f41fea
Merge branch 'main' into feature/stateful_url
jitu5 Apr 2, 2024
885ac55
Reset button status check removed
jitu5 Apr 3, 2024
dc9317d
Reset buton status test removed
jitu5 Apr 3, 2024
15c1113
Reset button status check revert
jitu5 Apr 3, 2024
2e74f1f
Reset button UI update
jitu5 Apr 3, 2024
bfebde2
code review suggestions for reset button status
jitu5 Apr 9, 2024
e93dbb5
Merge branch 'main' into feature/stateful_url
jitu5 Apr 9, 2024
9071420
state compare for nodeTypes
jitu5 Apr 9, 2024
4b016f9
On group filter click fix
jitu5 Apr 9, 2024
47f5aa1
Task to node mapping added
jitu5 Apr 10, 2024
4919374
All nodeTypes match UI labels with URLparmas
jitu5 Apr 10, 2024
5c038b1
Removed console.log
jitu5 Apr 10, 2024
778ace5
modified test as per query param name update
jitu5 Apr 12, 2024
ceacc74
Merge branch 'main' into feature/stateful_url
jitu5 Apr 12, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions RELEASE.md
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ Please follow the established format:

## Major features and improvements

- Extending stateful URLs with node filters and expand/collapse modular pipelines. (#1799)
- Introduce `--include-hooks` option and remove `--ignore-plugins` from cli commands. (#1818)
- Add Dataset Factory Patterns to Experiment Tracking. (#1824)

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -198,10 +198,7 @@ describe('Experiment Tracking', () => {
cy.get('.accordion__title--hyperlink').first().click();

// Assert after action
cy.location('search').should(
'eq',
`?pipeline_id=__default__&selected_name=${plotNameText}`
);
cy.location('search').should('contain', `?pid=__default__&sn=${plotNameText}`);
cy.__checkForText__(
'.pipeline-node--selected > .pipeline-node__text',
prettifyName(stripNamespace(plotNameText))
Expand Down
2 changes: 1 addition & 1 deletion cypress/tests/ui/flowchart/flowchart.cy.js
Original file line number Diff line number Diff line change
Expand Up @@ -190,7 +190,7 @@ describe('Flowchart DAG', () => {
// Assert after action
cy.get('.pipeline-warning__title')
.should('exist')
.and('have.text', `Oops, there's nothing to see here`);
.and('include.text', `Oops, there's nothing to see here`);
});

it('verifies that users can open and see the dataset statistics in the metadata panel for datasets. #TC-51', () => {
Expand Down
46 changes: 45 additions & 1 deletion cypress/tests/ui/flowchart/menu.cy.js
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ describe('Flowchart Menu', () => {

cy.location('search').should((queryParams) => {
expect(decodeURIComponent(queryParams).toLowerCase()).to.contain(
menuOptionValue.toLowerCase()
menuOptionValue.toLowerCase().replace(/ /g, '+')
);
});

Expand Down Expand Up @@ -170,4 +170,48 @@ describe('Flowchart Menu', () => {
.should('have.class', 'pipeline-nodelist__row__label--faded')
.should('have.class', 'pipeline-nodelist__row__label--disabled');
});

it('verifies that after checking node type URL should be updated with correct query params', () => {
const nodeToToggleText = 'Parameters';

// Alias
cy.get(`.pipeline-nodelist__row__checkbox[name=${nodeToToggleText}]`).as(
'nodeToToggle'
);

// Assert before action
cy.get('@nodeToToggle').should('not.be.checked');

// Action
cy.get('@nodeToToggle').check({ force: true });

// Assert after action
cy.url().should('include', 'parameters');
});

it('Verify that if the URL contains the nodeTag query parameter, the same parameter should be reflected on the UI.', () => {
const visibleRowLabel = 'Companies';
cy.visit(`/?tags=${visibleRowLabel}`);

// Alias
cy.get(`.pipeline-nodelist__row__checkbox[name=${visibleRowLabel}]`).as(
'nodeToToggle'
);

// Assert
cy.get('@nodeToToggle').should('be.checked');
});

it('Verify that if the URL contains the nodeType query parameter, the same parameter should be reflected on the UI.', () => {
const visibleRowLabel = 'Datasets';
cy.visit('/?types=data');

// Alias
cy.get(`.pipeline-nodelist__row__checkbox[name=${visibleRowLabel}]`).as(
'nodeToToggle'
);

// Assert
cy.get('@nodeToToggle').should('be.checked');
});
});
63 changes: 58 additions & 5 deletions src/components/flowchart-wrapper/flowchart-wrapper.js
Original file line number Diff line number Diff line change
Expand Up @@ -30,11 +30,13 @@ import {
errorMessages,
linkToFlowchartInitialVal,
localStorageFlowchartLink,
localStorageName,
params,
} from '../../config';
import { findMatchedPath } from '../../utils/match-path';
import { getKeyByValue } from '../../utils/get-key-by-value';
import { isRunningLocally } from '../../utils';
import { getKeyByValue, getKeysByValue } from '../../utils/object-utils';
import { isRunningLocally, mapNodeTypes } from '../../utils';
import { useGeneratePathname } from '../../utils/hooks/use-generate-pathname';
import './flowchart-wrapper.scss';

/**
Expand All @@ -55,10 +57,12 @@ export const FlowChartWrapper = ({
onUpdateActivePipeline,
pipelines,
sidebarVisible,
activePipeline,
}) => {
const history = useHistory();
const { pathname, search } = useLocation();
const searchParams = new URLSearchParams(search);
const { toSetQueryParam } = useGeneratePathname();

const [errorMessage, setErrorMessage] = useState({});
const [isInvalidUrl, setIsInvalidUrl] = useState(false);
Expand All @@ -78,6 +82,54 @@ export const FlowChartWrapper = ({
matchedFocusedNode,
} = findMatchedPath(pathname, search);

/**
* On initial load & when user switch active pipeline,
* sets the query params from local storage based on NodeType, tag, expandAllPipelines and active pipeline.
* @param {string} activePipeline - The active pipeline.
*/
const setParamsFromLocalStorage = (activePipeline) => {
ravi-kumar-pilla marked this conversation as resolved.
Show resolved Hide resolved
const localStorageParams = loadLocalStorage(localStorageName);
if (localStorageParams) {
const paramActions = {
pipeline: (value) => {
if (!searchParams.has(params.pipeline) && activePipeline) {
toSetQueryParam(params.pipeline, value.active || activePipeline);
}
},
tag: (value) => {
if (!searchParams.has(params.tags)) {
const enabledKeys = getKeysByValue(value.enabled, true);
enabledKeys && toSetQueryParam(params.tags, enabledKeys);
}
},
nodeType: (value) => {
if (!searchParams.has(params.types)) {
const disabledKeys = getKeysByValue(value.disabled, false);
// Replace task with node to keep UI label & the URL consistent
const mappedDisabledNodes = mapNodeTypes(disabledKeys);
disabledKeys && toSetQueryParam(params.types, mappedDisabledNodes);
}
},
flags: (value) => {
ravi-kumar-pilla marked this conversation as resolved.
Show resolved Hide resolved
if (!searchParams.has(params.expandAll)) {
toSetQueryParam(params.expandAll, value.expandAllPipelines);
}
},
};

for (const [key, value] of Object.entries(localStorageParams)) {
if (paramActions[key]) {
paramActions[key](value);
}
}
}
};

useEffect(() => {
setParamsFromLocalStorage(activePipeline);
// eslint-disable-next-line react-hooks/exhaustive-deps
}, [activePipeline]);

const resetErrorMessage = () => {
setErrorMessage({});
setIsInvalidUrl(false);
Expand Down Expand Up @@ -199,17 +251,17 @@ export const FlowChartWrapper = ({
resetErrorMessage();
}

if (matchedSelectedPipeline) {
if (matchedSelectedPipeline()) {
// Redirecting to a different pipeline is also handled at `preparePipelineState`
// to ensure the data is ready before being passed to here
redirectSelectedPipeline();
}

if (matchedSelectedNodeName || matchedSelectedNodeId) {
if (matchedSelectedNodeName() || matchedSelectedNodeId()) {
redirectToSelectedNode();
}

if (matchedFocusedNode) {
if (matchedFocusedNode()) {
redirectToFocusedNode();
}

Expand Down Expand Up @@ -312,6 +364,7 @@ export const mapStateToProps = (state) => ({
modularPipelinesTree: getModularPipelinesTree(state),
nodes: state.node.modularPipelines,
pipelines: state.pipeline.ids,
activePipeline: state.pipeline.active,
sidebarVisible: state.visible.sidebar,
});

Expand Down
87 changes: 85 additions & 2 deletions src/components/node-list/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@ import {
} from '../../actions/nodes';
import { useGeneratePathname } from '../../utils/hooks/use-generate-pathname';
import './styles/node-list.scss';
import { params, NODE_TYPES } from '../../config';

/**
* Provides data from the store to populate a NodeList component.
Expand Down Expand Up @@ -68,9 +69,16 @@ const NodeListProvider = ({
inputOutputDataNodes,
}) => {
const [searchValue, updateSearchValue] = useState('');
const [isResetFilterActive, setIsResetFilterActive] = useState(false);

const { toSelectedPipeline, toSelectedNode, toFocusedModularPipeline } =
useGeneratePathname();
const {
toSelectedPipeline,
toSelectedNode,
toFocusedModularPipeline,
toUpdateUrlParamsOnResetFilter,
toUpdateUrlParamsOnFilter,
toSetQueryParam,
} = useGeneratePathname();

const items = getFilteredItems({
nodes,
Expand Down Expand Up @@ -105,10 +113,50 @@ const NodeListProvider = ({
}
};

// To get existing values from URL query parameters
const getExistingValuesFromUrlQueryParams = (paramName, searchParams) => {
const paramValues = searchParams.get(paramName);
return new Set(paramValues ? paramValues.split(',') : []);
};

const handleUrlParamsUpdateOnFilter = (item) => {
const searchParams = new URLSearchParams(window.location.search);
const paramName = isElementType(item.type) ? params.types : params.tags;
const existingValues = getExistingValuesFromUrlQueryParams(
paramName,
searchParams
);

toUpdateUrlParamsOnFilter(item, paramName, existingValues);
};

// To update URL query parameters when a filter group is clicked
const handleUrlParamsUpdateOnGroupFilter = (
groupType,
groupItems,
groupItemsDisabled
) => {
if (groupItemsDisabled) {
// If all items in group are disabled
groupItems.forEach((item) => {
handleUrlParamsUpdateOnFilter(item);
});
} else {
// If some items in group are enabled
const paramName = isElementType(groupType) ? params.types : params.tags;
toSetQueryParam(paramName, []);
}
};

const onItemChange = (item, checked, clickedIconType) => {
if (isGroupType(item.type) || isModularPipelineType(item.type)) {
onGroupItemChange(item, checked);

// Update URL query parameters when a filter item is clicked
if (!clickedIconType) {
handleUrlParamsUpdateOnFilter(item);
}

if (isModularPipelineType(item.type)) {
if (clickedIconType === 'focus') {
if (focusMode === null) {
Expand Down Expand Up @@ -169,6 +217,13 @@ const NodeListProvider = ({
(groupItem) => !groupItem.checked
);

// Update URL query parameters when a filter group is clicked
handleUrlParamsUpdateOnGroupFilter(
groupType,
groupItems,
groupItemsDisabled
);

if (isTagType(groupType)) {
onToggleTagFilter(
groupItems.map((item) => item.id),
Expand Down Expand Up @@ -207,6 +262,32 @@ const NodeListProvider = ({
onToggleNodeSelected(null);
}
};

// Reset applied filters to default
const onResetFilter = () => {
onToggleTypeDisabled({ task: false, data: false, parameters: true });
onToggleTagFilter(
tags.map((item) => item.id),
false
);

toUpdateUrlParamsOnResetFilter();
};

// Helper function to check if NodeTypes is modified
const hasModifiedNodeTypes = (nodeTypes) => {
return nodeTypes.some(
(item) => NODE_TYPES[item.id]?.defaultState !== item.disabled
);
};

// Updates the reset filter button status based on the node types and tags.
useEffect(() => {
Copy link
Contributor

@rashidakanchwala rashidakanchwala Mar 29, 2024

Choose a reason for hiding this comment

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

I was thinking maybe we can simplify this

setIsResetFilterActive(false) --> whenever the 'Reset' button is clicked

setIsResetFilterActive(true) --> whenever any tag, or filter is clicked.

So we avoid the complex logic below.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think doing that will work if someone uses the UI to do filtering, but if someone directly changes the url, the reset button status should also watch out for the url.

Copy link
Contributor

Choose a reason for hiding this comment

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

another suggestion is to follow the KISS principal and keep the Reset button always on but since this is also design suggestion looping @stephkaiser

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ravi-kumar-pilla @rashidakanchwala @stephkaiser I understand that the logic to set correct reset button status is complex, but are we keeping it simple like always ON or keeping what I did ?

Copy link
Contributor

Choose a reason for hiding this comment

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

My suggestion is keeping it permanently enabled. Even when both the 'Reset' and reset filter are active, it doesn't alter the outcome. Many UI designs follow this convention, where the clear button remains visible even when the form is already empty. That's just my perspective, but would like to hear what others think.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In case if we keep always ON,
If the user clicks the reset button while the filter is already in the default state, the state will still be set to the same values. In this scenario, if the user is zoomed in on the flowchart, it will also be reset.

Choose a reason for hiding this comment

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

Hi team, thanks for flagging to me. I think its confusing to users if the Reset button is always there even after they press it, theres no way for uses to tell they are on the default/reset view or the customised filtered view unless they look at the url. I think the convention is that when the reset/clear button is visible that implies that changes have been made and it can be reset to default (like when you've applied a filter on a website, a clear button will appear).

I wanted to ask what the default view is? because in my opinion it should be without any filters applied (nodes, datasets, parameters). Otherwise it's a bit confusing as it implies to users that a filtered view is the default, unless we want to make a filtered view the default then the Reset button will not be visible when those filters are applied.

What do you all think?

Copy link
Contributor

Choose a reason for hiding this comment

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

The default view is set to 'Nodes', with 'Datasets' enabled and 'Parameters' disabled. Hence, we use 'Reset' instead of 'Clear' filters.

Since majority including design agree having 'Reset' enabled all the time might confuse users, I think maintaining the way as Jitendra had previously done seems ok. We can see how we can make it less complex.

Copy link
Contributor

Choose a reason for hiding this comment

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

sounds good. For me this is similar to a subscriber pattern. The Reset button status should subscribe to any changes in the query params. I am not sure of the technical feasibility in this ticket, but yes we can modify in future if need be.

Copy link

@stephkaiser stephkaiser Apr 3, 2024

Choose a reason for hiding this comment

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

Another thought - we also shouldn't rely on a colour change in the Reset text to indicate that a change has been applied as this doesn't work with colourblind users.

Design fixes for the Reset button below:

  • font size 13 (same as the Types and Tags label under Filters)
  • font colour default FFFFFF 55%, and on hover FFFFFF 85% (similar pattern for Search)
  • no underline as it's not a link
  • after user clicks on Reset, button should disappear and only reappear if user changes have been made

const isNodeTypeModified = hasModifiedNodeTypes(nodeTypes);
const isNodeTagModified = tags.some((tag) => tag.enabled);
setIsResetFilterActive(isNodeTypeModified || isNodeTagModified);
}, [tags, nodeTypes]);

useEffect(() => {
window.addEventListener('keydown', handleKeyDown);
return () => window.removeEventListener('keydown', handleKeyDown);
Expand All @@ -230,6 +311,8 @@ const NodeListProvider = ({
onItemChange={onItemChange}
focusMode={focusMode}
disabledModularPipeline={disabledModularPipeline}
onResetFilter={onResetFilter}
isResetFilterActive={isResetFilterActive}
/>
);
};
Expand Down
17 changes: 14 additions & 3 deletions src/components/node-list/node-list.js
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,8 @@ const NodeList = ({
onModularPipelineToggleExpanded,
focusMode,
disabledModularPipeline,
onResetFilter,
isResetFilterActive,
}) => {
return (
<div
Expand Down Expand Up @@ -79,9 +81,18 @@ const NodeList = ({
autoHide
hideTracksWhenNotNeeded
>
<h2 className="pipeline-nodelist-section__title">
<span>Filters</span>
</h2>
<div className="pipeline-nodelist-section__filters">
<h2 className="pipeline-nodelist-section__title">
<span>Filters</span>
</h2>
<button
disabled={!isResetFilterActive}
onClick={onResetFilter}
className="pipeline-nodelist-section__reset-filter"
>
Reset
</button>
</div>
<NodeListGroups
items={items}
groups={groups}
Expand Down
Loading