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.

Sign up for GitHub

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

Jump to bottom

UI: Add copy URI to navigator #5185

Merged
merged 3 commits into from
Feb 6, 2023
Merged

UI: Add copy URI to navigator #5185

merged 3 commits into from
Feb 6, 2023

Conversation

N-o-Z
Copy link
Member

@N-o-Z N-o-Z commented Feb 5, 2023 •
edited
Loading

Closes #5186

@N-o-Z N-o-Z added area/UI Improvements or additions to UI include-changelog PR description should be included in next release changelog team/versioning-engine Team versioning engine labels Feb 5, 2023
@N-o-Z N-o-Z marked this pull request as ready for review February 5, 2023 15:31
@N-o-Z
Copy link
Member Author

N-o-Z commented Feb 5, 2023

image

image

eladlachmi
eladlachmi approved these changes Feb 5, 2023
View reviewed changes
Copy link
Contributor

@eladlachmi eladlachmi left a comment

Choose a reason for hiding this comment

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

LGTM

@N-o-Z N-o-Z self-assigned this Feb 5, 2023
@N-o-Z
Copy link
Member Author

N-o-Z commented Feb 5, 2023

@itaiad200 Why under uncommitted changes, the navigator displays " workspace"? Can we change that to show the real URI? (Does not affected the changes in this PR)

@itaiad200
Copy link
Contributor

@itaiad200 Why under uncommitted changes, the navigator displays " workspace"? Can we change that to show the real URI? (Does not affected the changes in this PR)

Fine by me. We have a long standing technical debt to unify the 3 options of objects browsing (object, uncommitted, compare). The workspace header thing is one of the caveats of having 3 different browsing options.

itaiad200
itaiad200 requested changes Feb 6, 2023
View reviewed changes
Copy link
Contributor

@itaiad200 itaiad200 left a comment

Choose a reason for hiding this comment

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

Thanks for this addition! Just a single comment..
Also - it seems like the clipboard logo in the button is not centered, or is it just me?

webui/src/lib/components/repository/tree.jsx Outdated
Comment on lines 631 to 632
hidden={isPathToFile}
disabled={isPathToFile}/>
Copy link
Contributor

Choose a reason for hiding this comment

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

Why disable it when it's not a file? I think copying prefixes is valuable too.

Copy link
Member Author

Choose a reason for hiding this comment

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

We're disabling when it is a file - since the object viewer has already a copy and download buttons

@N-o-Z
Copy link
Member Author

N-o-Z commented Feb 6, 2023

Thanks for this addition! Just a single comment.. Also - it seems like the clipboard logo in the button is not centered, or is it just me?

Fixed

@N-o-Z N-o-Z requested a review from itaiad200 February 6, 2023 08:28
@N-o-Z
Copy link
Member Author

N-o-Z commented Feb 6, 2023 •
edited
Loading

image

image

itaiad200
itaiad200 approved these changes Feb 6, 2023
View reviewed changes
Copy link
Contributor

@itaiad200 itaiad200 left a comment

Choose a reason for hiding this comment

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

LGTM

webui/src/lib/components/repository/tree.jsx Outdated
import Modal from "react-bootstrap/Modal";
import { useAPI } from "../../hooks/api";
import ButtonGroup from "react-bootstrap/ButtonGroup";
Copy link
Contributor

Choose a reason for hiding this comment

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

Please remove

webui/src/pages/repositories/repository/changes.jsx
@@ -258,7 +258,6 @@ const ChangesBrowser = ({repo, reference, prefix, onSelectRef, }) => {
<span className="float-start">
{(delimiter !== "") && (
<URINavigator path={prefix} reference={reference} repo={repo}
relativeTo={`${reference.id} workspace`}
Copy link
Contributor

Choose a reason for hiding this comment

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

You sure we want to remove the rerference.id too?

Copy link
Member Author

Choose a reason for hiding this comment

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

relativeTo is not needed, once it is empty the URI nevigator provides the lakefs URI using the reference id as in the Objects view

@N-o-Z N-o-Z merged commit 3fe853d into master Feb 6, 2023
@N-o-Z N-o-Z deleted the ui/add-copy-url-to-navigator branch February 6, 2023 12:00
@rmoff rmoff mentioned this pull request Feb 14, 2023
Merged
This was referenced Feb 22, 2023
Merged
Merged
This was referenced Mar 6, 2023
Merged
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@itaiad200 itaiad200 itaiad200 approved these changes

@eladlachmi eladlachmi eladlachmi approved these changes

@ozkatz ozkatz Awaiting requested review from ozkatz

Assignees

@N-o-Z N-o-Z

Labels
area/UI Improvements or additions to UI include-changelog PR description should be included in next release changelog team/versioning-engine Team versioning engine
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

UI: Add copy URL button to the navigator
3 participants
@N-o-Z @itaiad200 @eladlachmi