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

Move the Expand all modular pipelines toggle from the settings panel #1858

Merged
merged 14 commits into from
Apr 22, 2024

Conversation

SajidAlamQB
Copy link
Contributor

@SajidAlamQB SajidAlamQB commented Apr 11, 2024

Description

Related to: #1752

Research highlighted the need to make Expand all modular pipelines toggle more discoverable to users. It is currently in the settings panel which most users do not use often, and will be moved to the task bar in this PR.

Development notes

Removed from settings menu:

image

Expand modular pipelines:
image

image

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: Sajid Alam <sajid_alam@mckinsey.com>
Signed-off-by: Sajid Alam <sajid_alam@mckinsey.com>
Signed-off-by: Sajid Alam <sajid_alam@mckinsey.com>
@SajidAlamQB SajidAlamQB self-assigned this Apr 11, 2024
SajidAlamQB and others added 3 commits April 16, 2024 16:47
Signed-off-by: Sajid Alam <sajid_alam@mckinsey.com>
Signed-off-by: Sajid Alam <sajid_alam@mckinsey.com>
@SajidAlamQB SajidAlamQB marked this pull request as ready for review April 16, 2024 15:51
@SajidAlamQB SajidAlamQB requested a review from Huongg April 17, 2024 08:57
Signed-off-by: Sajid Alam <sajid_alam@mckinsey.com>
@jitu5
Copy link
Contributor

jitu5 commented Apr 17, 2024

@SajidAlamQB Please remove "General" label from setting modal as new Figma design.

Screenshot 2024-04-17 at 11 57 47 a m

@jitu5
Copy link
Contributor

jitu5 commented Apr 17, 2024

@SajidAlamQB On collapse/expand icon click, Please update URL query params expandAllPipelines. I added this functionality in stateful URL PR in settings-modal.js

SajidAlamQB and others added 2 commits April 17, 2024 14:17
Signed-off-by: Sajid Alam <sajid_alam@mckinsey.com>
@jitu5
Copy link
Contributor

jitu5 commented Apr 17, 2024

@SajidAlamQB In settings-modal.js as we are not updating URL you could remove code related to it.

import { useGeneratePathname } from '../../utils/hooks/use-generate-pathname';
...
const { toSetQueryParam } = useGeneratePathname();
...
 if (name === params.expandAll) {
      toSetQueryParam(params.expandAll, value);
    }
...
toSetQueryParam,

Signed-off-by: Sajid Alam <sajid_alam@mckinsey.com>
Copy link
Contributor

@jitu5 jitu5 left a comment

Choose a reason for hiding this comment

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

LGTM! 👍🏼

Signed-off-by: Sajid Alam <sajid_alam@mckinsey.com>
@stephkaiser
Copy link

Hey Sajid!

Thanks for this work! I've done some design QA with this link - https://6620ef0fc18fff36ef74b586--kedro-viz-expand-all.netlify.app/?types=nodes,datasets&expandAllPipelines=false. Please see my comments below:

I can see the url updates from false to true when the pipeline is expanded or collapsed, nice!

  • The flowchart is empty and showing an error message so I can't test the button to see if the pipeline expands or collapses.
  • The expand/collapse button icon should follow the same default and hover states pattern as the download button in the utility bar - so white-900 (#C6CACE) by default and on hover it should be FFFFFF 100%. Currently this pattern only applies when the button is on 'Expand'. And the same for light mode as well.
  • For the tooltip, we should have a lowercase P for pipelines, so "Expand pipelines" and "Collapse pipelines"
  • Settings modal -
    -- The Settings title and settings names should be in medium font weight and FFFFFF 85%
    -- Toggle 'On' labels should be in blue (#00E3FF)
    -- Missing the Kedro-Viz version text, which should be aligned with the buttons
    -- Button text is now 'Save and apply', and disabled button state colour is FFFFFF 30%
    -- Padding between the button and the settings need to be increased, they should be 64px apart

Let me know if anything is unclear :)

@stephkaiser stephkaiser self-requested a review April 18, 2024 12:56
Copy link
Contributor

@Huongg Huongg left a comment

Choose a reason for hiding this comment

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

Amazing job @SajidAlamQB 😄 I've tested it locally and the expand/collapse button works as expected. The code also looks good to me, Just one small comment from me but no blocker.

SajidAlamQB and others added 3 commits April 19, 2024 12:12
Signed-off-by: Sajid Alam <sajid_alam@mckinsey.com>
Signed-off-by: Sajid Alam <sajid_alam@mckinsey.com>
@SajidAlamQB
Copy link
Contributor Author

  • Settings modal -
    -- The Settings title and settings names should be in medium font weight and FFFFFF 85%

Hey @stephkaiser I've made all the changes except the one quoted above, setting it to medium font with FFFFFF 85% had no visible change so I've kept it as is.

Copy link
Contributor

@rashidakanchwala rashidakanchwala left a comment

Choose a reason for hiding this comment

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

Awesome, LGTM. thanks Sajid!

@SajidAlamQB SajidAlamQB merged commit 92860cd into main Apr 22, 2024
5 checks passed
@SajidAlamQB SajidAlamQB deleted the feat/move-expand-all-modular-piplines-from-settings branch April 22, 2024 09:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Move the 'Expand all modular pipelines' toggle from the settings panel.
5 participants