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

Share citation copy button #530

Merged
merged 11 commits into from
Jan 8, 2025
Merged

Share citation copy button #530

merged 11 commits into from
Jan 8, 2025

Conversation

orlinmalkja
Copy link
Contributor

@orlinmalkja orlinmalkja self-assigned this Nov 21, 2024
@paulpestov
Copy link
Collaborator

There is a discussion about this feature here #470.

@paulpestov
Copy link
Collaborator

The design of the button should be more seamless. Check out the implementation of it here in GitHub https://github.com/subugoe/tido/pull/530/files. If you look at the file path at the top of the list you will discover the small copy icon. Upon click it turns into a small check icon.

@orlinmalkja
Copy link
Contributor Author

Okay, I have another question: Do we still show the confirmation message 'Copied successfully' below the button ?

@paulpestov
Copy link
Collaborator

I'd say yes

@paulpestov
Copy link
Collaborator

The tooltip should be positioned absolutely. It should not move othr things in the view. Also the copy icon should be placed inline behind the text.

export const dropdown =
'<svg xmlns="http://www.w3.org/2000/svg" width="1em" height="1em" viewBox="0 0 16 16"><path fill="currentColor" fill-rule="evenodd" d="M1.646 4.646a.5.5 0 0 1 .708 0L8 10.293l5.646-5.647a.5.5 0 0 1 .708.708l-6 6a.5.5 0 0 1-.708 0l-6-6a.5.5 0 0 1 0-.708"/></svg>';
export const copy =
'<svg class="w-6 h-6 text-gray-800 dark:text-white" aria-hidden="true" xmlns="http://www.w3.org/2000/svg" width="24" height="24" fill="currentColor" viewBox="0 0 24 24"><path fill-rule="evenodd" d="M18 3a2 2 0 0 1 2 2v10a2 2 0 0 1-2 2h-1V9a4 4 0 0 0-4-4h-3a1.99 1.99 0 0 0-1 .267V5a2 2 0 0 1 2-2h7Z" clip-rule="evenodd"/><path fill-rule="evenodd" d="M8 7.054V11H4.2a2 2 0 0 1 .281-.432l2.46-2.87A2 2 0 0 1 8 7.054ZM10 7v4a2 2 0 0 1-2 2H4v6a2 2 0 0 0 2 2h7a2 2 0 0 0 2-2V9a2 2 0 0 0-2-2h-3Z" clip-rule="evenodd"/></svg>';
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why is everything reformatted? Also please don't add tailwind classes here. Use them on the outer element. Also look at the other icon svg. They have 1em as width/height. This is for using font-size on the outer element to control the size of the icon.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am not sure, why everything is shown as reformatted. I actually only added the extra copy icon. Anyway in one of my latest commits I restored the icons as they are in develop branch and then just added my icon.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Please check my comment above, the width/height should be 1em.

@paulpestov
Copy link
Collaborator

I still have a few points:

  • The icon should sit vertically centered, right now its slightly above the text line
  • When you switch to the checkmark icon, the text line should not jump, I think this is due to a different icon size
  • The "tooltip" text should sit at the bottom center of the icon and not at bottom left of the text line (again, just chck the copy button from Github)
  • The icon feels too heavy, maybe we should try some outlined icon like this: https://icones.js.org/collection/all?s=copy&icon=mingcute:copy-2-line (you can just copy the SVG)

} else {
// when there is one panel containing content and metadata views
const panelContainingMetadata = panels.filter((panel) =>
panel.label.toLowerCase().includes("metadata")
Copy link
Collaborator

Choose a reason for hiding this comment

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

This line breaks TIDO when testing with EUPT config. The reason is that you try to parse a label value. Please find a better way. We should never parse user given values in generic products!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@paulpestov thank you for the hint! I selected the metadata view by conditionally filtering the views using the connector id


return matches !== null;
}

function getMetadataView(panels) {
let panelMetadata = panels.filter(
(panel) => panel.label.toLowerCase() === "metadata"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Here the same, user value.

@paulpestov
Copy link
Collaborator

The points I stated previously are done 👍

@orlinmalkja
Copy link
Contributor Author

@paulpestov I think I fixed the selection part of the metadata view and centered the tooltip below the share button!

@orlinmalkja orlinmalkja merged commit b697eba into develop Jan 8, 2025
2 checks passed
@orlinmalkja orlinmalkja deleted the share-citation-copy-button branch January 8, 2025 14:21
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.

2 participants