[KED-2328] Add code panel #346

bru5 · 2021-01-07T17:51:40Z

Description

Adds a code panel to the metadata sidebar.

Development notes

Adds the code panel component
Uses highlight.js for code highlighting
Handles both light and dark highlighting themes
Improves the responsive approach so that the chart moves and scales to fit
Adds a feature flag named code

QA notes

Testing requires a live Kedro project running through the viz API.

The code feature flag must be enabled before testing.

When a function node is clicked and code is available from the API, the code panel toggle will appear at the top of the metadata panel. Other node types do not show the toggle.

Clicking the toggle will open the panel and show highlighted code.

Test at various browser sizes and with different sidebar configurations, as the chart will responsively scale to fit the space available until a minimum chart size.

The code panel is intentionally a fixed width, if code is wider than the panel it can be scrolled horizontally.

When the clicked node is changed or removed, the code panel will close itself.

Test using both themes.

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

Legal notice

I acknowledge and agree that, by checking this box and clicking "Submit Pull Request":
I submit this contribution under the Apache 2.0 license and represent that I am entitled to do so on behalf of myself, my employer, or relevant third parties, as applicable.
I certify that (a) this contribution is my original creation and / or (b) to the extent it is not my original creation, I am authorised to submit this contribution on behalf of the original creator(s) or their licensees.
I certify that the use of this contribution as authorised by the Apache 2.0 license does not violate the intellectual property rights of anyone else.

studioswong · 2021-01-11T09:55:53Z

src/components/flowchart/index.js

@@ -213,7 +218,10 @@ export class FlowChart extends Component {
        const margin = 500;
        this.zoomBehaviour.translateExtent([
          [-sidebarWidth / scale - margin, -margin],
-          [width + margin + metaSidebarWidth / scale, height + margin]
+          [
+            width + margin + (metaSidebarWidth + codeSidebarWidth) / scale,


With the introduction of the 2 new props to calculate the flowchart size, it is worth to add a test for flowchart to test the recalculation of the scale with different chartsize values.

src/components/metadata/index.js

studioswong · 2021-01-11T12:41:03Z

src/components/metadata/index.js

+        {metadata && (
+          <>
+            <div className="pipeline-metadata__header-toolbox">
+              {showCodeSwitch && (


With the introduction of the codeSwitch there is a need to should add / update the metadata tests to ensure the toggle button only shows up for task type nodes.

Agreed I'll be adding some additional tests around this.

studioswong · 2021-01-11T12:53:17Z

Tested with the data from the spaceflight project, nice work @bru5 ! Love the automatic resizing of the code panel for different screen sizes.

I've added a few comments with regards to adding tests for the newly introduced toggle button and screen resizing - overall it looks good!

richardwestenra · 2021-01-11T14:35:45Z

src/components/metadata/metadata-code-toggle.js

+    <input
+      id="pipeline-metadata__code-toggle-input"
+      className="pipeline-metadata__code-toggle-input"
+      type="checkbox"
+      checked={showCode}
+      disabled={!hasCode}
+      onChange={onChange}
+    />
+    <label
+      className={modifiers('pipeline-metadata__code-toggle-label', {
+        checked: hasCode && showCode
+      })}
+      htmlFor="pipeline-metadata__code-toggle-input">
+      Show Code
+    </label>


@GabrielComymQB The toggle icon implies to me that it will stay flipped open until you explicitly close it again, so that next time you click a node, it will still be open. Instead, it flips back to closed when you deselect a node or hide the metadata panel.

I think that either it should stay open until you close it, or else the icon should be changed to something that doesn't imply the same level of permanence (e.g. something like this: ◀️)

Agreed I've discussed with @GabrielComymQB and he is currently testing the branch out so we should discuss after.

I'm not keen on the flow with it closing all the time but probably fine for first flagged release, I've suggested moving towards using separate toolbar buttons for each panel going forwards to allow the user more control.

ok cool so we'll look at this later? I'll make a ticket for it.

src/components/metadata/styles/metadata-code.scss

src/components/flowchart/index.js

src/components/metadata/index.js

src/components/metadata/styles/metadata.scss

src/components/sidebar/sidebar.scss

src/config.js

src/selectors/layout.js

richardwestenra

I'd still like some styling improvements on the toggle switch focus state, but overall you've addressed all my comments, great job, thanks! This is awesome.

If the language isn't recognised then hljs will fail without this check.

# Conflicts: # package-lock.json # package.json # src/components/flowchart/index.js # src/config.js # src/selectors/layout.js # src/selectors/layout.test.js

studioswong · 2021-02-11T17:28:02Z

src/components/metadata/styles/metadata-code.scss

+.pipeline-metadata-code__title {
+  flex-grow: 1;
+  margin: 0;
+  margin: 2.1em 36px 1.8em 36px;


This should align with using the same unit (em ) - @richardwestenra ?

Why do you say that? I think this makes sense to use em for the vertical margin (which is based on the relative font-size), but use px for the horizontal margin (which is based on the container's padding, which is based on the overall container width, which is handled in pixels).

Yeah exactly it's a bit nuanced I know but my approach to spacing usually depends on context. I tend to use em for vertical spacing as that's naturally related to typography and vertical rhythm. But really it's not a big deal in practice anymore, but I still find it's still a good way of expressing the intent.

Thanks for the clarification @richardwestenra - I am quite confused as to our current preference of unit within the codebase - are we using px or em? My understanding is that it is either em or px as a consistent unit within the codebase, and if we are making exceptions for context then it's very easy for us to fall into the trap of individual interpretation?

That's fair - I've added this to the list of things to add to the styleguide in KED-2413.

We typically use em for font-sizes, and sizing things that might want to be set relative to the font-size - e.g. text margin-top/bottom, line-height, etc. This is partly because it is more accessible, and partly because it allows us to set the font-size in one place (i.e. on the kedro class) and then if we want to change later then everything else will update based on that value (e.g. if you change the font-size of a heading, you'll want its vertical margin/line-height to increase accordingly).

We typically use px and % for layout - i.e. things that are more dependant on the size of the viewport than on the size of the font.

I see, these 2 types of cases makes sense. In this case can we generally categorize to use px / % for all layout-related fields ( i.e fields that would stay constant regardless of font sizing), and em for all font related / font-affected values?

studioswong · 2021-02-11T17:29:02Z

src/components/metadata/styles/metadata-code.scss

+.pipeline-metadata-code pre {
+  display: inline-block;
+  margin: 0;
+  padding: 0 36px 36px;


same as above - should be 3.6em?

As above, I'm not sure that this needs to be in em, because in this case it's not based on font-size but on overall panel width, which is handled in px like most of our layout. There's an argument to be made that literally everything should be done in em, but that's not where we're currently at.

Exactly that thanks for clarifying @richardwestenra

richardwestenra · 2021-02-12T13:56:36Z

src/components/metadata/styles/metadata-code.scss

+  margin: 0;
+  margin: 2.1em 36px 1.8em 36px;


Something like this:

Suggested change

margin: 0;

margin: 2.1em 36px 1.8em 36px;

margin: 2.1em (36px + $metadata-sidebar-width-open) 1.8em 36px;

Delete margin: 0; (as it's overridden/duplicate and hence duplicate

This needs 400px added to the 36px on the right side otherwise long titles will overrun the panel rather than wrapping:

Note that in this image I've reduced the opacity of the metadata panel.

Good point I've was able to refactor the positioning approach to flexbox with the same scrolling behaviour, so this should also resolve this potential issue too.

richardwestenra · 2021-02-12T13:59:25Z

src/components/metadata/styles/metadata-code.scss

+
+.pipeline-metadata-code__code {
+  position: absolute;
+  top: 7.1em;


Using a fixed value for the top will break when the node name is very long. This needs to be changed so that it works with a variable title height.

Sure I managed to refactor this to flexbox and retain the scrolling behaviour, so this should now be ready should the title change from being a fixed string.

richardwestenra · 2021-02-12T14:02:46Z

src/components/metadata/styles/metadata-code.scss

+.pipeline-metadata-code pre {
+  display: inline-block;
+  margin: 0;
+  padding: 0 36px 36px;


As above, I'm not sure that this needs to be in em, because in this case it's not based on font-size but on overall panel width, which is handled in px like most of our layout. There's an argument to be made that literally everything should be done in em, but that's not where we're currently at.

richardwestenra

Not done with my review but I noticed these two glaring issues

src/components/metadata/styles/metadata-code.scss

It wasn't picked up on git commit for some reason?

richardwestenra

🚢🚢🚢🚢

# Release 3.9.0 ## Major features and improvements - Add code panel (#346) - Improve view panning behaviour when a node is selected (#356, #363, #370, #373, #374) - Improve layout performance for large graphs (#343) - Save tag state to localStorage (#353) ## Bug fixes and other changes - Improve graph layout code quality, performance and docs (#347) - Update docs to remind on compatibility of Kedro-Viz 3.8.0 with Kedro 0.17 (#367) - Update dependencies (#360, #364, #369) - Fix failing CircleCI build on Windows (#365, #366) - Refactor node-list-row CSS, fixing hover and focus states (#355, #358, #362) - Update iconography (#357, #359) - Fix missing indicator Chrome zoom bug (#349) - Fix sidebar border/box-shadow CSS rules (#351) - Fix server.py to work with versions >0.17 and update contributing docs (#348) - Fix errors when scrolling with empty pipeline (#342) - Ignore coverage on some branches and fix e2e tests (#345) - Fix icon-button tooltips on mobile (#344) - Update SVG-Crowbar to fix errors (#339) - Update contributing docs for new dev server (#341)

bru5 added 10 commits December 15, 2020 18:04

[KED-2328] Add highlight.js

8d25877

[KED-2328] Add MetaDataCode and MetaCodeToggle components

2569ac5

[KED-2328] Add code panel features to MetaData panel behind new flag

2533dee

[KED-2328] Improve copy button on metadata panel

c5856e9

[KED-2328] Default to not expanded when values change on MetaDataList

531b54f

[KED-2328] Adaptive chart sizing to fit space

07d64a3

[KED-2328] Adaptive chart sizing to fit space

d2e834b

[KED-2328] Add yaml language to code panel

1393089

Merge branch 'main' into feature/code-panel

883c2d6

[KED-2328] Improve responsive approach

2ccfaf4

bru5 added the Type: Feature label Jan 7, 2021

bru5 requested review from richardwestenra, studioswong, yetudada and comym January 7, 2021 17:51

bru5 added 3 commits January 8, 2021 11:53

Merge branch 'main' into feature/code-panel

735d800

[KED-2212] Update tests for code panel

e7ca799

[KED-2212] Update dependency highlight.js

5276b0d

studioswong reviewed Jan 11, 2021

View reviewed changes

src/components/metadata/index.js Show resolved Hide resolved

studioswong reviewed Jan 11, 2021

View reviewed changes

src/components/metadata/index.js Show resolved Hide resolved

studioswong reviewed Jan 11, 2021

View reviewed changes

bru5 added 3 commits January 11, 2021 17:23

Merge branch 'main' into feature/code-panel

d7e0c13

[KED-2212] Avoid sidebar tooltip z-index issue

ddbd5db

[KED-2212] Add shadow when sidebars overlap

7f52bae

richardwestenra suggested changes Jan 12, 2021

View reviewed changes

bru5 added 3 commits January 14, 2021 11:22

[KED-2212] Fix variable error on sidebar

27a8968

[KED-2212] Add padding to code block on code panel

e55a624

[KED-2212] Use screenReaderOnly mixin on code toggle

abea22f

bru5 requested a review from richardwestenra January 15, 2021 10:32

richardwestenra approved these changes Jan 15, 2021

View reviewed changes

richardwestenra and others added 5 commits January 18, 2021 16:36

Protect against unrecognised language errors

0608a05

If the language isn't recognised then hljs will fail without this check.

Merge branch 'main' into feature/code-panel

27a31a3

# Conflicts: # package-lock.json # package.json # src/components/flowchart/index.js # src/config.js # src/selectors/layout.js # src/selectors/layout.test.js

[KED-2328] Update view behaviour for code panel

b512030

[KED-2328] Add tests for extents in flowchart

7c420a0

[KED-2328] Improve code panel toggle focus style

866fc14

bru5 requested review from richardwestenra and studioswong February 10, 2021 12:49

[KED-2328] Use earlier test for getSidebarWidth

3661158

richardwestenra assigned bru5 Feb 11, 2021

bru5 added 2 commits February 11, 2021 11:15

[KED-2421] Reformat all JavaScript in /src with Prettier

e6d4225

Merge main into feature/code-panel

b500796

studioswong reviewed Feb 11, 2021

View reviewed changes

richardwestenra reviewed Feb 12, 2021

View reviewed changes

richardwestenra suggested changes Feb 12, 2021

View reviewed changes

[KED-2328] Refactor code panel positioning

7ab4ec1

richardwestenra reviewed Feb 12, 2021

View reviewed changes

src/components/metadata/styles/metadata-code.scss Outdated Show resolved Hide resolved

richardwestenra suggested changes Feb 12, 2021

View reviewed changes

src/components/metadata/styles/metadata-code.scss Outdated Show resolved Hide resolved

bru5 requested a review from richardwestenra February 12, 2021 17:50

bru5 and others added 3 commits February 15, 2021 10:07

[KED-2328] Fix code toggle contrast

9857e3a

Combine CSS rulesets

f4908c3

Format SCSS with stylelint

d30ca83

It wasn't picked up on git commit for some reason?

richardwestenra approved these changes Feb 15, 2021

View reviewed changes

bru5 merged commit a53a269 into main Feb 15, 2021

bru5 deleted the feature/code-panel branch February 15, 2021 10:52

This was referenced Feb 17, 2021

3.9.0 #371

Merged

Add note about 'code' flag to readme #372

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[KED-2328] Add code panel #346

[KED-2328] Add code panel #346

bru5 commented Jan 7, 2021 •

edited

Loading

studioswong Jan 11, 2021 •

edited

Loading

studioswong Jan 11, 2021

bru5 Jan 14, 2021

studioswong commented Jan 11, 2021

richardwestenra Jan 11, 2021

bru5 Jan 14, 2021

richardwestenra Feb 11, 2021

richardwestenra left a comment •

edited

Loading

studioswong Feb 11, 2021

richardwestenra Feb 12, 2021

bru5 Feb 12, 2021

studioswong Feb 15, 2021

richardwestenra Feb 15, 2021 •

edited

Loading

studioswong Feb 15, 2021

studioswong Feb 11, 2021

richardwestenra Feb 12, 2021

bru5 Feb 12, 2021

richardwestenra Feb 12, 2021

bru5 Feb 12, 2021

richardwestenra Feb 12, 2021

bru5 Feb 12, 2021

richardwestenra Feb 12, 2021

richardwestenra left a comment

richardwestenra left a comment

	margin: 0;
	margin: 2.1em 36px 1.8em 36px;
	margin: 2.1em (36px + $metadata-sidebar-width-open) 1.8em 36px;

[KED-2328] Add code panel #346

[KED-2328] Add code panel #346

Conversation

bru5 commented Jan 7, 2021 • edited Loading

Description

Development notes

QA notes

Checklist

Legal notice

studioswong Jan 11, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

studioswong commented Jan 11, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

richardwestenra left a comment • edited Loading

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

richardwestenra Feb 15, 2021 • edited Loading

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

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

richardwestenra left a comment

Choose a reason for hiding this comment

richardwestenra left a comment

Choose a reason for hiding this comment

bru5 commented Jan 7, 2021 •

edited

Loading

studioswong Jan 11, 2021 •

edited

Loading

richardwestenra left a comment •

edited

Loading

richardwestenra Feb 15, 2021 •

edited

Loading