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

Conversation

jitu5
Copy link
Contributor

@jitu5 jitu5 commented Aug 14, 2023

Description

This resolves case below for the Pretty name feature 1252

When the feature is turned on, the sidebar search will only allow the user to search for pretty-named things. For example, if one searches for data_processing.preprocessed_shuttles nothing will be returned, even though preprocessed shuttles exists and is the same thing. The same is true in the opposite direction.

Development notes

Now it works for both the type of search terms Pretty name and non Pretty name if it exist.

Screenshot 2023-08-14 at 7 21 26 p m Screenshot 2023-08-14 at 7 22 07 p m

Checklist

  • Read the contributing guidelines
  • Opened this PR as a 'Draft Pull Request' if it is work-in-progress
  • Updated the documentation to reflect the code changes
  • Added new entries to the RELEASE.md file
  • Added tests to cover my changes

@jitu5 jitu5 added Enhancement Javascript Pull requests that update Javascript code labels Aug 14, 2023
@jitu5 jitu5 self-assigned this Aug 14, 2023
@jitu5 jitu5 linked an issue Aug 15, 2023 that may be closed by this pull request
1 task
Signed-off-by: Jitendra Gundaniya <jitendra_gundaniya@mckinsey.com>
Signed-off-by: Jitendra Gundaniya <jitendra_gundaniya@mckinsey.com>
Copy link
Contributor

@ravi-kumar-pilla ravi-kumar-pilla left a comment

Choose a reason for hiding this comment

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

I have added few comments. Please have a look. Thank you

src/selectors/nodes.js Show resolved Hide resolved
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?

@@ -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.

Copy link
Contributor

@ravi-kumar-pilla ravi-kumar-pilla left a comment

Choose a reason for hiding this comment

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

I have left few comments and none of them is a blocking change, so approving it now

@tynandebold
Copy link
Member

Looking really good so far! The results are returning the way they should, and that's the main point of the PR, so well done there.

I am noticing one style bit that's off. On your branch, the search results aren't always highlighted like they should be. Notice the results aren't highlighted here:

image

It should look something like this:

image

This could be tricky to solve though, because what's being highlighted is the same exact text string that the user has entered. Have a look though and see if you can get it.

@tynandebold tynandebold removed the request for review from yetudada August 16, 2023 20:01
@jitu5
Copy link
Contributor Author

jitu5 commented Aug 17, 2023

@tynandebold Now searched/matched text is being highlighted in both the cases. please let me know if anythings is missing or broken.

RELEASE.md Outdated Show resolved Hide resolved
Copy link
Member

@tynandebold tynandebold left a comment

Choose a reason for hiding this comment

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

Thanks for making that style change. It looks good to me now, and I'll approve.

I do think there's a little bit of odd UX wherein there can be a mismatch between what's returned in the sidebar and what's show in the flowchart. E.g. if prettyName is off and you search for Create mat you'll get back Create Matplot Chart, but in the flowchart the text of that node is create_matplotlib_chart.

I think this is ok for now. Let's see if anyone complains. The benefit is now you can search for both pretty and normal names. Nicely done!

@jitu5
Copy link
Contributor Author

jitu5 commented Aug 17, 2023

@tynandebold For this E.g. if prettyName is off and you search for Create mat you'll get back Create Matplot Chart, but in the flowchart the text of that node is create_matplotlib_chart.

Before the changes I did just now, it was returning correct text same as flowchart but it want highlighting because the enter text is pretty name (Create mat ) and returned text is non pretty name (create_matplotlib_chart).
But as you said we will see if anyone complains.

Co-authored-by: Tynan DeBold <thdebold@gmail.com>
@jitu5 jitu5 merged commit 713cd77 into main Aug 17, 2023
5 checks passed
@jitu5 jitu5 deleted the fix/pretty_name_search branch August 17, 2023 12:57
@tynandebold tynandebold mentioned this pull request Aug 17, 2023
5 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Enhancement Javascript Pull requests that update Javascript code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Improve the Pretty name feature
3 participants