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

Add tooltip to shareable urls copy button #1614

Merged
merged 6 commits into from
Nov 3, 2023

Conversation

vladimir-mck
Copy link
Contributor

@vladimir-mck vladimir-mck commented Oct 27, 2023

Description

Updates to tooltip component and adding it copy to clipboard and displaying it as feedback.

Resolves #1555

Development notes

Screenshot 2023-10-27 at 17 16 00

QA notes

This can be tested by following the instructions from this PR and clicking the copy button after creating the url.

A quick and dirty way of testing - go to shareable-url-modal component and comment all the html except the url result form and click the button.

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

…osition

Signed-off-by: Vladimir <vladimir_nikolic@external.mckinsey.com>
Copy link
Member

@tynandebold tynandebold left a comment

Choose a reason for hiding this comment

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

The output of this is looking good. We need to make sure that this implementation is changed across the app though, as it says in #1555:

implement the new component globally across the kedro-viz tool.

Can you please implement this for the command copier in the metadata panel?

Screenshot 2023-10-30 at 11 49 04

Signed-off-by: Vladimir <vladimir_nikolic@external.mckinsey.com>
Copy link
Member

@tynandebold tynandebold left a comment

Choose a reason for hiding this comment

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

Looking good. One small comment about the padding being a little bit off.

Also seeing some new deprecation warnings in the terminal when running npm start:

image

Lastly, and you don't need to do this in this PR: I try to advocate for sorting CSS and React properties in alphabetical order so it's easier for other devs to work through the codebase.

position: relative;
width: max-content;
max-width: calc(50vw - 150px);
padding: 12px 20px;
Copy link
Member

Choose a reason for hiding this comment

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

Looking at the design, I think these values are off a little bit:

image

Copy link
Contributor Author

@vladimir-mck vladimir-mck Nov 1, 2023

Choose a reason for hiding this comment

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

Oh I see the warning now. It's there only for a moment and I didn't see it.

I do agree sorting React props makes sense but it doesn't for css props. It would be easier to read if the are sorted by display, layout, appearance and text props.

Potentially we can add a script that auto-sorts the css props for us by given rules.

Signed-off-by: Vladimir <vladimir_nikolic@external.mckinsey.com>
Copy link
Contributor

@ravi-kumar-pilla ravi-kumar-pilla left a comment

Choose a reason for hiding this comment

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

LGTM !!

@tynandebold
Copy link
Member

Can we one line to the release notes about this as well?

Signed-off-by: Vladimir <vladimir_nikolic@external.mckinsey.com>
@vladimir-mck vladimir-mck merged commit 6c063bf into main Nov 3, 2023
1 check passed
@vladimir-mck vladimir-mck deleted the enhancment/1555-clipboard-btn-redisgn branch November 3, 2023 08:37
@rashidakanchwala rashidakanchwala mentioned this pull request Nov 16, 2023
5 tasks
rashidakanchwala added a commit that referenced this pull request Nov 17, 2023
This is minor release with big backend refactoring work and some bug fixes.

Bug fixes and other changes
Refactor flowchart dataclasses to pydantic base models. (Refactor Flowchart models from dataclass to pydantic base models #1565)
Fix dataset factory patterns in Experiment Tracking. (Fix dataset factory patterns in Experiment Tracking #1588)
Update demo-project to use OmegaConfigLoader. (Update demo project to use OmegaConfigLoader #1590)
Improve feedback for copy to clipboard feature. (Add tooltip to shareable urls copy button #1614)
Ensure Kedro-Viz works when hosted on a URL subpath. (Fix: Kedro-Viz doesn't work when hosted via a URL subpath #1621)
Bump fastapi upper bounds. (Bump FAST API upper bounds #1634)
Fix shareable URL modal to appear across the app. (Make shareable URL modal open globally across the app.  #1639)
Add Kedro-Viz CLI command deprecation warning. (Add kedro viz deprecation warning for CLI #1641)
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.

Copy to Clipboard component implementation
3 participants