Fix: lambda python functions not rendering correctly on flowchart #851

tynandebold · 2022-05-16T11:14:58Z

Description

Fixes #817.

Development notes

I'm no longer using _strip_tags in the backend, so the <> come through to the frontend. I've create a replaceMatches() utility function and added that to the node-list-row.js file to ensure both <lambda> and <partial> will render via dangerouslySetInnerHTML.

QA notes

I've added a lambda node so this can be tested. Open the Gitpod URL and expand the Ingestion modular pipeline. You'll see the lambda function there.

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

Signed-off-by: Tynan DeBold <[email protected]>

studioswong

It looks like this PR contains the work for this PR that is not merged in main yet - would be great if you could remove those changes in this PR to avoid confusion. Thank you!

…x/lambda-python-functions-on-flowchart Signed-off-by: Tynan DeBold <[email protected]>

tynandebold · 2022-05-16T14:15:57Z

It looks like this PR contains the work for this PR that is not merged in main yet - would be great if you could remove those changes in this PR to avoid confusion. Thank you!

Done now.

Signed-off-by: Tynan DeBold <[email protected]>

antonymilne · 2022-05-16T17:01:07Z

This looks right on the tree view and metadata side panel to me but looks kind of funky when I search for it...

Outside the scope of this PR, but related: did we ever get a ticket created to change these partial names to something more descriptive? As per the discussion in #692.

antonymilne · 2022-05-16T17:01:51Z

RELEASE.md

@@ -6,6 +6,10 @@ Please follow the established format:
 - Include the ID number for the related PR (or PRs) in parentheses
 -->

+## Bug fixes and other changes
+
+- Fix lambda Python functions not rendering correctly on flowchart. (#851)


Suggested change

- Fix lambda Python functions not rendering correctly on flowchart. (#851)

- Fix lambda and partial Python functions not rendering correctly on flowchart. (#851)

The same fix here will work for both cases :)

Nice one. Updated!

antonymilne · 2022-05-16T17:05:31Z

src/utils/index.js

+};
+
+export const replaceMatches = (str) => {
+  if (str && str.length > 0) {


Curious: what is this conditional for here? I sort of understand the zero-length string case but not the case where bool(str) would evaluate to False - when does that occur?

If you don't have that check you'd get this error: TypeError: Cannot read property 'length' of undefined. I've changed it now to this str?.length > 0 which does the same thing: first checks if there's a string, and if so, proceed to find the length.

Signed-off-by: Tynan DeBold <[email protected]>

tynandebold · 2022-05-16T17:57:12Z

This looks right on the tree view and metadata side panel to me but looks kind of funky when I search for it...

Outside the scope of this PR, but related: did we ever get a ticket created to change these partial names to something more descriptive? As per the discussion in #692.

Good catch on the search. I need to update it, but it's a bit tricky since we're putting these partial and lambda names between < > symbols, which really conflicts with HTML, since that will render an HTML element.

Can we use something else instead of these to make it easier for ourselves, like a ~ or something? Or is < > convention?

…x/lambda-python-functions-on-flowchart Signed-off-by: Tynan DeBold <[email protected]>

antonymilne · 2022-05-16T21:11:26Z

Good catch on the search. I need to update it, but it's a bit tricky since we're putting these partial and lambda names between < > symbols, which really conflicts with HTML, since that will render an HTML element.

Can we use something else instead of these to make it easier for ourselves, like a ~ or something? Or is < > convention?

I'm afraid <> is quite a strong convention since this is how Python names anonymous functions:

If <> is too awkward then I would just not have any special character there at all, i.e. lambda. This would be fine I think.

studioswong

Thanks for tackling this.

I haven't reviewed the code yet, but on checking out the branch and poking around it seems that this breaks the search functionality in general:

This is due to having taken out the dangerouslySetInnerHTML which is needed to implement the bold highlighting during search.

Signed-off-by: Tynan DeBold <[email protected]>

tynandebold · 2022-05-17T11:53:20Z

package/kedro_viz/models/graph.py

-def _strip_tags(name: str) -> str:
-    return name.replace("<", "&lt;").replace(">", "&gt;")


I think the solve for this is to leave the <> in place in the backend and then change it to use the HTML entities only where we need it in the frontend.

tynandebold · 2022-05-17T11:53:57Z

src/components/node-list/node-list-row.js

@@ -124,7 +131,9 @@ const NodeListRow = memo(
                'pipeline-nodelist__row__label--disabled': disabled,
              }
            )}
-            dangerouslySetInnerHTML={{ __html: label }}
+            dangerouslySetInnerHTML={{
+              __html: replaceMatches(label, stringsToReplace),


Replace the <> here so we can still use dangerouslySetInnerHTML without a problem, specifically when searching.

Signed-off-by: Tynan DeBold <[email protected]>

tynandebold · 2022-05-17T12:03:27Z

src/components/search-list/search-list.scss

    width: 20px;
-    height: 20px;
+    z-index: 1;


You couldn't click the X icon to clear the search. This fixes that.

tynandebold · 2022-05-18T13:56:47Z

README.md

@@ -35,6 +35,7 @@ Kedro-Viz is an interactive development tool for building data science pipelines
 - 🔎 Highly interactive, filterable and searchable
 - 🔬 Focus mode for modular pipeline visualisation
 - 📊 Rich metadata side panel to display parameters, plots, etc.
+- 📊 Supports [plotly](https://plotly.com/javascript/) charts. 


@rashidakanchwala curious why this made it in here?

Signed-off-by: Tynan DeBold <[email protected]>

…thub.com/kedro-org/kedro-viz into fix/lambda-python-functions-on-flowchart Signed-off-by: Tynan DeBold <[email protected]>

…x/lambda-python-functions-on-flowchart Signed-off-by: Tynan DeBold <[email protected]>

tynandebold · 2022-05-18T16:34:32Z

On the main branch, searching for something with < doesn't work. See here:

I tried to fix that in this ticket but it turned into a bigger issue. I'll create a separate bug report for that and leave this one here.

Update: related ticket here: #854

…:kedro-org/kedro-viz into fix/lambda-python-functions-on-flowchart

Signed-off-by: Tynan DeBold <[email protected]>

…thub.com/kedro-org/kedro-viz into fix/lambda-python-functions-on-flowchart Signed-off-by: Tynan DeBold <[email protected]>

…x/lambda-python-functions-on-flowchart Signed-off-by: Tynan DeBold <[email protected]>

rashidakanchwala

thanks @tynandebold . approving it since we will look at the <> bug in another ticket :)

studioswong

The PR is definitely in a much better shape now!

one small thing I noticed ( sorry to be a pain) - when searching for the lambda function with a <, the search results displayed gets weird ( some characters gets wiped out):

I think this could potentially be tied to having replaced <lambda with <partial> ? ( though I am not sure at all - this might require a bit of further poke around to find a workaround for that.)

tynandebold · 2022-05-20T15:09:13Z

The PR is definitely in a much better shape now!

one small thing I noticed ( sorry to be a pain) - when searching for the lambda function with a <, the search results displayed gets weird ( some characters gets wiped out):

I think this could potentially be tied to having replaced <lambda with <partial> ? ( though I am not sure at all - this might require a bit of further poke around to find a workaround for that.)

Good eye. I think that's to be expected at this point, which will be solved by the solution for this bug ticket.

In the current version, if you have a <lambda> function and search for <lam, nothing actually shows up. In this case, if you do the same, at least you get a search result, even though the highlighting isn't exactly correct. I would say returning the result you're showing in your screenshot is better than not returning anything at all.

If you're comfortable with that, would be great if we can approve this now and iterate on the fix in the linked ticket above.

studioswong

Just saw the ticket you mentioned above, all good 👍

May I suggest to add in an end-2-end test in the upcoming PR for #854 to ensure we capture any errors with the search case for Lambda functions ( since this seems to be a use case that could be easily prone to errors)? 😆

Given that we are looking into releasing soon, it might be worth to release first before merging this in so this can be released alongside the fix for the search (#854 ) as one complete fix? ( totally up to the team)

antonymilne

Definitely good to get this merged, thanks for doing all the iteration on it!

If it turns out the search thing is hard to fix then honestly I think it's fine if we just drop the <> altogether and call it plain lambda or partial everywhere. Also the number of people who will be searching for a lambda function in the is very small. So would be good to fix but not a biggy at all I think.

tynandebold · 2022-05-20T16:10:02Z

Definitely good to get this merged, thanks for doing all the iteration on it!

If it turns out the search thing is hard to fix then honestly I think it's fine if we just drop the <> altogether and call it plain lambda or partial everywhere. Also the number of people who will be searching for a lambda function in the is very small. So would be good to fix but not a biggy at all I think.

Good to know. I'll add this comment to the context of the other ticket I created. I do think it'll be a pain to solve and we'd be wise to just drop <> all together, especially as it's probably a very small slice of users searching for it, as you said.

tynandebold added 2 commits May 12, 2022 13:28

update demo project to 0.18.1

ac1bd25

Signed-off-by: Tynan DeBold <[email protected]>

fix the bug

1bf863d

Signed-off-by: Tynan DeBold <[email protected]>

tynandebold requested review from studioswong, rashidakanchwala and antonymilne May 16, 2022 11:14

tynandebold requested review from yetudada and limdauto as code owners May 16, 2022 11:14

update release notes

74564b2

Signed-off-by: Tynan DeBold <[email protected]>

tynandebold removed the request for review from yetudada May 16, 2022 11:16

studioswong reviewed May 16, 2022

View reviewed changes

Merge branch 'main' of https://github.com/kedro-org/kedro-viz into fi…

48b530a

…x/lambda-python-functions-on-flowchart Signed-off-by: Tynan DeBold <[email protected]>

tynandebold requested a review from studioswong May 16, 2022 14:15

tynandebold marked this pull request as draft May 16, 2022 14:39

better implementation

4d8dec4

Signed-off-by: Tynan DeBold <[email protected]>

tynandebold marked this pull request as ready for review May 16, 2022 14:46

antonymilne reviewed May 16, 2022

View reviewed changes

tynandebold added 3 commits May 16, 2022 18:38

fix search highlighting

8c216a7

Signed-off-by: Tynan DeBold <[email protected]>

cleaner

acd397a

Signed-off-by: Tynan DeBold <[email protected]>

update releaes notes

af2eba7

Signed-off-by: Tynan DeBold <[email protected]>

Merge branch 'main' of https://github.com/kedro-org/kedro-viz into fi…

1f6eb76

…x/lambda-python-functions-on-flowchart Signed-off-by: Tynan DeBold <[email protected]>

studioswong suggested changes May 17, 2022

View reviewed changes

update

f2923b8

Signed-off-by: Tynan DeBold <[email protected]>

tynandebold requested review from studioswong and antonymilne and removed request for studioswong May 17, 2022 11:50

tynandebold commented May 17, 2022

View reviewed changes

z-index search close icon

8692956

Signed-off-by: Tynan DeBold <[email protected]>

tynandebold commented May 17, 2022

View reviewed changes

added to readme

54a18d5

tynandebold commented May 18, 2022

View reviewed changes

tynandebold added 3 commits May 18, 2022 17:28

update

450a1af

Signed-off-by: Tynan DeBold <[email protected]>

Merge branch 'fix/lambda-python-functions-on-flowchart' of https://gi…

4a271ba

…thub.com/kedro-org/kedro-viz into fix/lambda-python-functions-on-flowchart Signed-off-by: Tynan DeBold <[email protected]>

Merge branch 'main' of https://github.com/kedro-org/kedro-viz into fi…

9df69c8

…x/lambda-python-functions-on-flowchart Signed-off-by: Tynan DeBold <[email protected]>

rashidakanchwala and others added 7 commits May 18, 2022 17:43

undo readme changes

c152a64

Merge branch 'fix/lambda-python-functions-on-flowchart' of github.com…

b3923fb

…:kedro-org/kedro-viz into fix/lambda-python-functions-on-flowchart

undo readme changes

7b08e6f

undo plotly readme changes

bebec6c

undo readme-2

0bfe837

cleanup

45240cc

Signed-off-by: Tynan DeBold <[email protected]>

Merge branch 'fix/lambda-python-functions-on-flowchart' of https://gi…

abbea45

…thub.com/kedro-org/kedro-viz into fix/lambda-python-functions-on-flowchart Signed-off-by: Tynan DeBold <[email protected]>

tynandebold mentioned this pull request May 18, 2022

Unable to search for a <lambda Python function in the sidebar #854

Closed

1 task

Merge branch 'main' of https://github.com/kedro-org/kedro-viz into fi…

1027bd9

…x/lambda-python-functions-on-flowchart Signed-off-by: Tynan DeBold <[email protected]>

rashidakanchwala approved these changes May 19, 2022

View reviewed changes

tynandebold mentioned this pull request May 20, 2022

Release v4.6.0 #856

Merged

5 tasks

studioswong reviewed May 20, 2022

View reviewed changes

studioswong approved these changes May 20, 2022

View reviewed changes

Merge branch 'main' into fix/lambda-python-functions-on-flowchart

7c25f75

antonymilne approved these changes May 20, 2022

View reviewed changes

tynandebold merged commit 3760c8a into main May 20, 2022

tynandebold deleted the fix/lambda-python-functions-on-flowchart branch May 20, 2022 16:10

antonymilne mentioned this pull request May 25, 2022

Fix lambda function in demo-project #866

Merged

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix: lambda python functions not rendering correctly on flowchart #851

Fix: lambda python functions not rendering correctly on flowchart #851

tynandebold commented May 16, 2022 •

edited

Loading

studioswong left a comment •

edited

Loading

tynandebold commented May 16, 2022

antonymilne commented May 16, 2022

antonymilne May 16, 2022

tynandebold May 16, 2022

antonymilne May 16, 2022

tynandebold May 16, 2022

tynandebold commented May 16, 2022

antonymilne commented May 16, 2022 •

edited

Loading

studioswong left a comment

tynandebold May 17, 2022

tynandebold May 17, 2022

tynandebold May 17, 2022

tynandebold May 18, 2022

tynandebold commented May 18, 2022 •

edited

Loading

rashidakanchwala left a comment

studioswong left a comment

tynandebold commented May 20, 2022

studioswong left a comment

antonymilne left a comment

tynandebold commented May 20, 2022

	- Fix lambda Python functions not rendering correctly on flowchart. (#851)
	- Fix lambda and partial Python functions not rendering correctly on flowchart. (#851)

		def _strip_tags(name: str) -> str:
		return name.replace("<", "<").replace(">", ">")

Fix: lambda python functions not rendering correctly on flowchart #851

Fix: lambda python functions not rendering correctly on flowchart #851

Conversation

tynandebold commented May 16, 2022 • edited Loading

Description

Development notes

QA notes

Checklist

studioswong left a comment • edited Loading

Choose a reason for hiding this comment

tynandebold commented May 16, 2022

antonymilne commented May 16, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tynandebold commented May 16, 2022

antonymilne commented May 16, 2022 • edited Loading

studioswong left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tynandebold commented May 18, 2022 • edited Loading

rashidakanchwala left a comment

Choose a reason for hiding this comment

studioswong left a comment

Choose a reason for hiding this comment

tynandebold commented May 20, 2022

studioswong left a comment

Choose a reason for hiding this comment

antonymilne left a comment

Choose a reason for hiding this comment

tynandebold commented May 20, 2022

tynandebold commented May 16, 2022 •

edited

Loading

studioswong left a comment •

edited

Loading

antonymilne commented May 16, 2022 •

edited

Loading

tynandebold commented May 18, 2022 •

edited

Loading