[KED-2872] Create global toolbar UI #576

tynandebold · 2021-09-29T13:35:03Z

Description

This PR adds a new, left-aligned, vertical global toolbar, which is the first step in what will eventually become the full Experiment Tracking view and feature.

Development notes

I've added the global toolbar, moved the relevant icon buttons to it, added the Kedro logo and the two icon buttons for the respective pages that will be created soon, and created new tests and updated the existing ones.

I'm not thrilled with the naming for some of my class names, e.g. pipeline-global-control-toolbar. Since we're now introducing a new paradigm into the application with Experiment Tracking, perhaps we no longer need to prefix all class names with pipeline-, because not everything is a pipeline anymore. And that's what I did in an earlier commit, with something like global-control-toolbar but then reverted back to the old pattern. Curious to hear other thoughts!

Lastly, I haven't added anything to the release markdown file because I don't think we've aligned on when we should add in those notes.

QA notes

The changes implemented in this PR can be viewed here.

@hamzaoza I wasn't sure if the icons in the global toolbar need to be enlarged. In Zeplin, they look slightly larger than what's in the current primary toolbar. For now, I've left them the same size as what they were in their previous location. Let me know if that's incorrect.

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.

…s to global toolbar

…into feature/create-global-toolbar-ui#ked-2872

…m to the global sidebar

studioswong · 2021-09-29T14:17:50Z

Thanks for the great work @tynandebold , I had a skim through and a play around with the deployed link and it looks absolutely cracking 💯

Re: pipeline- prefix, indeed this is definitely something in the pipeline for us to get rid of ( the pipeline- prefix has existed historically and has been raised multiple times to be gotten rid of over the past months - thanks for raising this concern as well 👍 ! ) This should ideally be dealt with in this ticket.

I'll take a deeper dive into the code and let you know of comments, but so far it looks awesome!

hamzaoza · 2021-09-29T14:18:46Z

Great to see this starting to come to life 🚀 and already looking great 😄

You're right, the icons in the global navigation do look small. They should be 24x24px both on the global navigation and utility bar so probably need to be bumped up a little.

Two minor questions:

Can we remove the tooltip from the top two icons on hover (pipeline and experiments tracking)? It looks a bit strange and would prefer the full square background hover interactions that we have in the designs.
Is the plan to change the hide menu icon and the order of icons in the utility bar in this PR or a separate PR?

tynandebold · 2021-09-29T14:23:03Z

Great to see this starting to come to life 🚀 and already looking great 😄

You're right, the icons in the global navigation do look small. They should be 24x24px both on the global navigation and utility bar so probably need to be bumped up a little.

Two minor questions:

Can we remove the tooltip from the top two icons on hover (pipeline and experiments tracking)? It looks a bit strange and would prefer the full square background hover interactions that we have in the designs.

Is the plan to change the hide menu icon and the order of icons in the utility bar in this PR or a separate PR?

Cool, so 24px everywhere then. I'll update.

Yes, can remove the tooltip and add the hover interaction. I'll also update the hide menu icon and change the order. Didn't see that those had changed. Thanks for pointing that out.

tynandebold · 2021-09-29T14:26:17Z

Thanks for the great work @tynandebold , I had a skim through and a play around with the deployed link and it looks absolutely cracking 💯

Re: pipeline- prefix, indeed this is definitely something in the pipeline for us to get rid of ( the pipeline- prefix has existed historically and has been raised multiple times to be gotten rid of over the past months - thanks for raising this concern as well 👍 ! ) This should ideally be dealt with in this ticket.

I'll take a deeper dive into the code and let you know of comments, but so far it looks awesome!

Oh nice! Didn't realize there was another ticket for that. That's great.

Also, there's a huge amount of changes in my package-lock. Not sure why. All I've done in clone the project and npm install. Maybe a fresh install hasn't been done in a while and then committed though. Let me know what should be done there.

studioswong

Overall code looks great! 🥇

My main comment is simply to remove the visible prop for both the themeButton and the settingsButton, as well as the relevant test - this is a legacy prop that would have no use case now.

This also calls for us to delete all instances of visible.themeBtn and visible.settingsBtn ( particularly in our initial state) as there is no use of those fields anymore - this can be done via a seperate ticket as tech improvement.

Once the prop is removed, happy to merge this in to move this forward 👍

studioswong · 2021-09-29T15:02:44Z

src/components/minimap-toolbar/minimap-toolbar.scss

@@ -10,10 +10,9 @@
 }

 .pipeline-minimap-toolbar {
-  display: block;
+  @extend %listReset;


src/components/global-toolbar/global-toolbar.test.js

src/components/global-toolbar/index.js

src/components/primary-toolbar/primary-toolbar.test.js

…state values

hamzaoza · 2021-10-01T10:38:34Z

Thanks for this @tynandebold this is 🔥 🔥 🔥 and it's looking great! I'm basing my comments on this so let me know if this is an old build and I need to look at a new version and my comment's no longer apply.

Two minor points:

The tab of the selected page i.e. tree view or experiments tracking should have the active colour on it. Right now the highlight shows on hover.
The global bar should slide out from view when you click the hide menu icon. Have a play with the hide menu icon on this example

and thanks for cleaning up some of the inconsistencies in the toolbar, it has made a huge difference already 😄

tynandebold · 2021-10-01T11:44:41Z

Thanks for this @tynandebold this is 🔥 🔥 🔥 and it's looking great! I'm basing my comments on this so let me know if this is an old build and I need to look at a new version and my comment's no longer apply.

Two minor points:

The tab of the selected page i.e. tree view or experiments tracking should have the active colour on it. Right now the highlight shows on hover.

The global bar should slide out from view when you click the hide menu icon. Have a play with the hide menu icon on this example

and thanks for cleaning up some of the inconsistencies in the toolbar, it has made a huge difference already 😄

My pleasure @hamzaoza! That's the correct link to look at. Responding to your points:

We'll wire that up when we're actually able to switch between two pages.
Oh good catch. I missed this. If this is the intended behavior though I think it defeats the purpose of having a global toolbar. Wouldn't we always want the user to be able to navigate to the other page or control the theme/settings no matter where they are? If the global toolbar collapses with the sidebar it'll take an extra click to be able to perform any of these actions.

FYI: we may elect to get this merged into our experiment-tracking base branch now and continue to tweak behavior as we go along.

cc @studioswong

studioswong

great stuff - thanks for all the changes and congrats on a great first PR on Kedro-viz!!! 💯

@hamzaoza I share the same views as Tynan on the global toolbar for it to be in place on hide menu, mainly given the easier navigation between the experiment and flowchart page for those users who normally keeps their sidebar hidden ( which our localstate memorizes this pattern and hence reloads the collapsed sidebar everytime the page reloads.)

As Tynan mentioned, this is merged into a feature branch so can definitely be revisited before this feature is released.

* [KED-2872] Create global toolbar UI (#576) * adds in the global toolbar component; moves settings and theme buttons to global toolbar * adds new logo icons for kedro, pipeline, and experiments and adds them to the global sidebar * adds new test file for global-toolbar; updates primary-toolbar tests * updates icon styles and order; removes unnecessary tests and initial state values * removes two unneeded App propTypes * KED-2875: Set up routing infrastructure and component placeholders for experiment tracking page (#582) * setup of experiment router route * setup flowchart Wrapper and refactored old wrapper component * refadctored sidebar and added experiment route * Set up runlist sidebar, runsDetails and basic routing * delete unneeded flowchart wrapper test * modify route to runsList and refactor to use queryparams for routing * receive unneccessary change to wrapper scss file * added comment for experiment details wrapper * added memoryRouter to test and delete visible prop * set up useRunIdsFromUrl hook and refactor to use fragments * change from flowchart to flowChart * [KED-2882] Create experiment tracking sidebar UI (#587) * create sidebar runs card; add some simple tests; add necessary colors * add a collapsable container and necessary tests; small renaming and cleanup * add comments * update default accordion header value; remove unneeded margins * capitalization * update light bg color on run card; use && instead of ternary; add border-bottom to last run card; remove some placeholder content * update runs-list-card tests * [KED-2849] & [KED-2851] Set up GraphQL mock server (#604) * set up graphql schema * set up schema and mocks for dev server, along with apollo mock provider and client * update to runsList component to take in mock data * updates to mocks * further updates to mock setup * update to comments in mocks * updates to data type of run-list-card * added fetch library as polyfill for lib-test import * added new props to runs-list * set up schema.graphql and updates to schema * added fetch back in to enable testJS import test to pass on CI * added fetch and createHttpLink * [KED-2883] create experiment details UI (#605) * create a RunMetadata component and style accordingly * container and single-run styling * add the show more button to the notes area * update button font * starting on the metric details work * expand accordion component to include another layout and different size titles * ui driven by common mock-data file * shared accordions positioned, styled, and collapsing correctly * better mock data * scroll metadata and metric together * move RunMetrics code into own component; style tweaks * solid refactor of the RunMetrics component * some renamning; add some comments * refactor dataset accordions section after new data shape agreed upon * fix failing RunListCard test * add a show less button; add more accordion tests * add tests for RunMetadata and RunDataset components * add headingClassName to RunsList accordion * element alignment * update trackingData shape and refactor RunDataset code accordingly * [KED-2911] Implement the comparison-view journey (#619) * update mocks to align with schema from miro; begin runs list selection to display metadata * toggle comparison view working and updating RunsList; reusing Sidebar and PrimaryToolBar components * single run selection/deselection working * comparison run selection/deselection working * style adjustments; test with dummy graphql server * fix failing tests * create a Switch component and wire it up; remove the dummy icon in the PrimaryToolbar * connect to deployed dummy graphql endpoint * empty commit to see if pytest works * pr fixes; create a apollo hooks wrapper function to better transition between query results * prevent single run deselection in comparison view mode * more robust test for RunsListCard component * add the currently designed empty runs screen Co-authored-by: Rashida Kanchwala <rashida.kanchwala@quantumblack.com> * update http link to use actal graphql endpoint * update schema and queries with new showDiff variable (#624) * update schema and queries with new showDiff variable * updated schema and queries per PR comments * updates to queries and tracking details component * added queries back in * added changes to component level for graphql data * further modify naming of value * KED-2944: integrate the real API with experiment tracking frontend (#630) * update http link to use actal graphql endpoint * updates to queries and tracking details component * added queries back in * added changes to component level for graphql data * [KED-2954] Format experiment tracking timestamp (#625) * create a toHumanReadableTime date util and use in experiment tracking * fix failing tests * graphql endpoint back to local viz url * update formatting for timestamp in toHumanReadableTime function * simplify toHumanReadableTime logic * update lock file Co-authored-by: Tynan DeBold <thdebold@gmail.com> Co-authored-by: Rashida Kanchwala <rashida.kanchwala@quantumblack.com>

tynandebold added 4 commits September 27, 2021 17:48

adds in the global toolbar component; moves settings and theme button…

563d04a

…s to global toolbar

Merge branch 'main' of https://github.com/quantumblacklabs/kedro-viz …

507e4a2

…into feature/create-global-toolbar-ui#ked-2872

adds new logo icons for kedro, pipeline, and experiments and adds the…

aaff24f

…m to the global sidebar

adds new test file for global-toolbar; updates primary-toolbar tests

5b6f701

tynandebold requested review from limdauto, studioswong and rashidakanchwala September 29, 2021 13:35

studioswong reviewed Sep 29, 2021

View reviewed changes

tynandebold added 2 commits September 30, 2021 12:40

updates icon styles and order; removes unnecessary tests and initial …

bac8e14

…state values

removes two unneeded App propTypes

17b62eb

tynandebold requested a review from studioswong September 30, 2021 15:09

studioswong approved these changes Oct 1, 2021

View reviewed changes

tynandebold merged commit b088160 into experiment-tracking/single-view-journey-1 Oct 1, 2021

tynandebold deleted the feature/create-global-toolbar-ui#ked-2872 branch October 1, 2021 12:58

studioswong mentioned this pull request Nov 22, 2021

Experiment Tracking Front End set of features #640

Merged

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[KED-2872] Create global toolbar UI #576

[KED-2872] Create global toolbar UI #576

tynandebold commented Sep 29, 2021

studioswong commented Sep 29, 2021

hamzaoza commented Sep 29, 2021

tynandebold commented Sep 29, 2021 •

edited

Loading

tynandebold commented Sep 29, 2021

studioswong left a comment

studioswong Sep 29, 2021

hamzaoza commented Oct 1, 2021

tynandebold commented Oct 1, 2021 •

edited

Loading

studioswong left a comment

[KED-2872] Create global toolbar UI #576

[KED-2872] Create global toolbar UI #576

Conversation

tynandebold commented Sep 29, 2021

Description

Development notes

QA notes

Checklist

Legal notice

studioswong commented Sep 29, 2021

hamzaoza commented Sep 29, 2021

tynandebold commented Sep 29, 2021 • edited Loading

tynandebold commented Sep 29, 2021

studioswong left a comment

Choose a reason for hiding this comment

studioswong Sep 29, 2021

Choose a reason for hiding this comment

hamzaoza commented Oct 1, 2021

tynandebold commented Oct 1, 2021 • edited Loading

studioswong left a comment

Choose a reason for hiding this comment

tynandebold commented Sep 29, 2021 •

edited

Loading

tynandebold commented Oct 1, 2021 •

edited

Loading