kedro-org / kedro-viz

Visualise your Kedro data and machine-learning pipelines and track your experiments.
https://demo.kedro.org
Apache License 2.0
672 stars 110 forks source link

Retrieve viz url #1907

Closed Huongg closed 3 months ago

Huongg commented 4 months ago

Description

Fixes #1851 Design is here retrieve-url

Development notes

Screenshot 2024-05-21 at 16 48 53

QA notes

You can test the UI using Gitpod. I've implemented the logic prior to the check that validates whether the submitted platform is valid or not. As a result, you can use any fabricated names and URLs, the system will display an error message.

Checklist

jitu5 commented 4 months ago

@Huongg As we discuss we need to refactor or break down shareable-url-modal.js component as its getting bigger and difficult to read.

jitu5 commented 4 months ago

@Huongg When I was testing locally, For the first time after successful deployment I got below screen. Is it expected to have no close or cancel button on success screen ?

Screenshot 2024-05-22 at 10 11 34 a m

Also after refresh I opened publish popup again I got below screen, here also we don't have no close or cancel button. Thanks.

Screenshot 2024-05-22 at 10 17 41 a m
Huongg commented 4 months ago

@Huongg When I was testing locally, For the first time after successful deployment I got below screen. Is it expected to have no close or cancel button on success screen ?

Screenshot 2024-05-22 at 10 11 34 a m

Also after refresh I opened publish popup again I got below screen, here also we don't have no close or cancel button. Thanks. Screenshot 2024-05-22 at 10 17 41 a m

hey @jitu5 yes it is expected, according to the design here

Huongg commented 4 months ago

@Huongg As we discuss we need to refactor or break down shareable-url-modal.js component as its getting bigger and difficult to read.

hey agree, I've done some refactoring by separating the components and simplifying its logic so we don't return null too much. Please take a look when you have time @rashidakanchwala @jitu5

stephkaiser commented 4 months ago

thanks @Huongg!

Design QA notes below:

Huongg commented 4 months ago

Hey @stephkaiser thank you, I believed I have addressed all your design comments apart from the one that bolded, Im not sure which padding you mentioned here Multiple hosted link dropdown (next to link) - left side padding should be 20px

let me know if you spotted anything else

stephkaiser commented 4 months ago

Nice work! thank you very much @Huongg!

To clarify the one in bold - Multiple hosted link dropdown (next to link) - left side padding should be 20px - I was referring to this dropdown in the retrieving url modal, the left side padding of the dropdown should be 20px so that the text aligns with the text in the dropdown menu.

Screenshot 2024-05-28 at 16 14 26

Also, the same dropdown menus in light mode have a black border, can we remove this please?

Screenshot 2024-05-28 at 16 19 45 Screenshot 2024-05-28 at 16 19 57

Thank you for fixing the "padding is too much above the title, should be 48px". Can we do the same for the right section (with the form) as well so that it is also aligned with the title? Also the title should always be in white-0 colour (#FFFFFF 100%) to match the subtitles.

Screenshot 2024-05-28 at 16 30 29

And last one - would it be possible to move the 'or publish...' text to the next line so that it reads better? Implementation -

Screenshot 2024-05-28 at 16 28 33

Design -

Screenshot 2024-05-28 at 17 24 39

Thank you!

jitu5 commented 4 months ago

@Huongg I see underline on hover republish button, is it expected ?

Screenshot 2024-05-31 at 11 32 51 a m
stephkaiser commented 4 months ago

@Huongg I see underline on hover republish button, is it expected ? Screenshot 2024-05-31 at 11 32 51 a m

thanks @jitu5, the underline for the hover state on the button is fine thanks!

Huongg commented 4 months ago

thanks @stephkaiser I've just addressed the rest of your comment Step, Let me know if you're happy with all the design

stephkaiser commented 4 months ago

thanks @stephkaiser I've just addressed the rest of your comment Step, Let me know if you're happy with all the design

looks perfect, no more comments from me. Thank you so much Huong! 🙏

ravi-kumar-pilla commented 4 months ago

Hi @Huongg , Apologies for the delay in reviewing the PR. Most of the code looks good. I had a first pass and left few comments and questions. Please have a look. Thank you

ravi-kumar-pilla commented 3 months ago

Hi @Huongg , Thanks for addressing all the comments. There are some minor observations and I would like to know what the team thinks @stephkaiser @jitu5 -

  1. When closing the modal of published view - It changes the form before closing the modal (minor thing but seems like something is changing on the screen)
  2. When closing the modal of main content from published view - It changes the modal to publish view before closing (minor thing but seems like something is changing on the screen)
  3. When I click on republish, the form data gets filled based on the selected provider, but if I change the dropdown source, even if the provider is in localStorage, the form is not populated.

    Let me know if you need more info. Thank you

minor_close_issue

Huongg commented 3 months ago

Hi @Huongg , Thanks for addressing all the comments. There are some minor observations and I would like to know what the team thinks @stephkaiser @jitu5 -

  1. When closing the modal of published view - It changes the form before closing the modal (minor thing but seems like something is changing on the screen)
  2. When closing the modal of main content from published view - It changes the modal to publish view before closing (minor thing but seems like something is changing on the screen)
  3. When I click on republish, the form data gets filled based on the selected provider, but if I change the dropdown source, even if the provider is in localStorage, the form is not populated.

Let me know if you need more info. Thank you

minor_close_issue minor_close_issue

hey so for point 1 and 2, it has been a behaviour of the form, if we think this is not right then we can create a separate issue for it. And for point 3, when you're in the form, it does not read from the localStorage anymore so when you change to a different platform, it wont be re-populated

Huongg commented 3 months ago

FYI: link to the bug report for @ravi-kumar-pilla's comment 1 and 2: https://github.com/kedro-org/kedro-viz/issues/1936