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

Change dataset details page PID text to a input field with copy button #1051

Merged
merged 5 commits into from
Apr 24, 2023

Conversation

TheMeinerLP
Copy link
Contributor

@TheMeinerLP TheMeinerLP commented Apr 18, 2023

Description

On the detail page of a dataset is a PID. Normaly the user needs to select manually the PID. This PR adds a input field with simple copy option.

Motivation

To deliver more easy a dataset PID add a UX improvement.

Fixes:

  • Nothing

Changes:

  • UX improvement

Tests included/Docs Updated?

  • Included for each change/fix?
  • Passing? (Merge will not be approved unless this is checked)
  • Docs updated?
  • New packages used/requires npm install?
  • Toggle added for new features?
  • Requires update of SciCat backend API?

Before:
image

After:
image

Copy link
Contributor

@nitrosx nitrosx left a comment

Choose a reason for hiding this comment

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

Check the scicat token copy under the user settings page.
We should implement it as it is done there

@@ -66,7 +66,14 @@
</tr>
<tr>
<th>PID</th>
<td>{{ dataset.pid }}</td>
<td>
<mat-form-field class="full-width">
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of creating a full functional text box, can we just add the copy button, like is done under user settings with the scicat token

Copy link
Contributor

Choose a reason for hiding this comment

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

From the best practice point of view, especially if you are an accessibility enthusiast, I wouldn't recommend to use onClick in a span to make it a button.
But in our case, I think it is completely fine to follow the pattern what we already have for consistency, and you can add an aria attribute if you want to.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay i will check that from the settings page and improve the current code

@minottic
Copy link
Contributor

minottic commented Apr 21, 2023

after the changes, I would add a test, similar to this:

describe("#onCopy()", () => {
it("should copy the token to the users clipboard and dispatch a showMessageAction", () => {
const commandSpy = spyOn(document, "execCommand");
dispatchSpy = spyOn(store, "dispatch");
const message = new Message(
"SciCat token has been copied to your clipboard",
MessageType.Success,
5000
);
component.onCopy("test");
expect(commandSpy).toHaveBeenCalledTimes(1);
expect(commandSpy).toHaveBeenCalledWith("copy");
expect(dispatchSpy).toHaveBeenCalledTimes(1);
expect(dispatchSpy).toHaveBeenCalledWith(showMessageAction({ message }));
});
});
});

Very minor comment, I don't think the box "Toggle added for new features?" should be ticked in this case

@nitrosx nitrosx merged commit b5a3e1a into SciCatProject:master Apr 24, 2023
@TheMeinerLP TheMeinerLP deleted the feature/pid-copy-field branch April 24, 2023 13:53
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.

4 participants