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

Fix for sidebar search result based on Pretty name flag #1485

Merged
merged 8 commits into from
Aug 17, 2023
1 change: 1 addition & 0 deletions RELEASE.md
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ Please follow the established format:
- Fix incorrect rendering of datasets in modular pipelines. (#1439)
- Fix broken SVG/PNG exports in light theme. (#1463)
- Fix dataset and global toolbar error with standalone React component (#1351)
- Fix Sidebar search result based on Pretty name setting (#1252)
jitu5 marked this conversation as resolved.
Show resolved Hide resolved
- Fix `ImportError` as kedro-datasets is now lazily loaded (#1481).

# Release 6.3.4
Expand Down
7 changes: 6 additions & 1 deletion src/selectors/modular-pipelines.js
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,12 @@ export const searchTree = (
// if the child node is a leaf, simply search the leaf's name
// and add to the search result if there is a match.
const found = searchString(childNode.data.name, searchValue);
if (found) {
const foundOpposite = searchString(
Copy link
Contributor

Choose a reason for hiding this comment

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

[Nit] I feel opposite terminology not fitting but at the same time, I cannot come up with a suitable name here. @tynandebold any suggestions ?

Copy link
Member

Choose a reason for hiding this comment

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

I'm fine with it.

childNode.data.oppositeForPrettyName,
searchValue
);

if (found || foundOpposite) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This works in searching for both pretty and non-pretty names but the node list displayed is changed.

Have a look at the gif - The display names in the list gets changed based on search value i.e., I disabled pretty name flag and searched for a pretty name "Feat" and the node list shows Feature Engineering instead of feature_engineering. Though the search results return a value, we should push names based on the pretty flag I guess

Display_Name changes

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I checked current version on https://demo.kedro.org and this is happening before my changes. We can create a separate ticket for this if this behaviour is not as expected.

Copy link
Contributor

Choose a reason for hiding this comment

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

@tynandebold is this the expected behavior ?

Copy link
Member

Choose a reason for hiding this comment

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

No, that isn't expected. @jitu5 is right though, this isn't due to his change.

In the screen capture, what's returned when you search for feat when prettyName is off should be feature_engineering and not Feature Engineering.

Jitendra, can you please create a separate ticket to fix that?

foundChildren.push({
...childNode,
data: {
Expand Down
14 changes: 13 additions & 1 deletion src/selectors/nodes.js
Original file line number Diff line number Diff line change
Expand Up @@ -113,6 +113,15 @@ export const getNodeLabel = createSelector(
isPrettyName ? nodeName : nodeFullName
);

/**
ravi-kumar-pilla marked this conversation as resolved.
Show resolved Hide resolved
* Returns opposite node label based on if pretty name is turned on/off
*/
export const getOppositeForPrettyName = createSelector(
[getIsPrettyName, getNodeName, getNodeFullName],
(isPrettyName, nodeName, nodeFullName) =>
isPrettyName ? nodeFullName : nodeName
);

/**
* Returns formatted nodes as an array, with all relevant properties
*/
Expand Down Expand Up @@ -182,6 +191,7 @@ export const getNodeDataObject = createSelector(
getNodeDisabledTag,
getNodeTypeDisabled,
getNodeModularPipelines,
getOppositeForPrettyName,
],
(
nodeIDs,
Expand All @@ -193,12 +203,14 @@ export const getNodeDataObject = createSelector(
nodeDisabledNode,
nodeDisabledTag,
typeDisabled,
nodeModularPipelines
nodeModularPipelines,
oppositeForPrettyName
) =>
nodeIDs.reduce((obj, id) => {
obj[id] = {
id,
name: nodeLabel[id],
oppositeForPrettyName: oppositeForPrettyName[id],
type: nodeType[id],
icon: getShortType(nodeDatasetType[id], nodeType[id]),
modularPipelines: nodeModularPipelines[id],
Expand Down
28 changes: 28 additions & 0 deletions src/selectors/nodes.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ import {
getNodesWithInputParams,
getInputOutputNodesForFocusedModularPipeline,
getNodeLabel,
getOppositeForPrettyName,
} from './nodes';
import {
toggleTextLabels,
Expand Down Expand Up @@ -132,6 +133,33 @@ describe('Selectors', () => {
expect(nodeLabels[nodeId]).toEqual(nodePrettyName);
});
});

describe('getOppositeForPrettyName', () => {
it('returns opposite node labels with full name when pretty name is turned off', () => {
const nodes = getVisibleNodes(mockState.spaceflights);
const nodeId = nodes[0].id;
const nodePrettyName = nodes[0].name;
const newMockState = reducer(
mockState.spaceflights,
toggleIsPrettyName(false)
);
const nodeLabels = getOppositeForPrettyName(newMockState);

expect(nodeLabels[nodeId]).toEqual(nodePrettyName);
});

it('returns opposite node labels with pretty name when pretty name is turned on', () => {
const nodes = getVisibleNodes(mockState.spaceflights);
const nodeId = nodes[0].id;
const nodeFullName = nodes[0].fullName;
const newMockState = reducer(
mockState.spaceflights,
toggleIsPrettyName(true)
);
const nodeLabels = getOppositeForPrettyName(newMockState);
expect(nodeLabels[nodeId]).toEqual(nodeFullName);
});
});
});

describe('getNodeData', () => {
Expand Down