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

feat: Add a copy URI icon #211

Merged
merged 12 commits into from
Jun 7, 2021
Merged

feat: Add a copy URI icon #211

merged 12 commits into from
Jun 7, 2021

Conversation

HagerDakroury
Copy link
Collaborator

@HagerDakroury HagerDakroury commented Jun 3, 2021

Checklist

  • I indicated which issue (if any) is closed with this PR using closing keywords
  • I only changed lines related to my PR (no bulk reformatting)
  • I indicated the source and check the license if I re-use code, or I did not re-use code
  • I made my best to solve only one issue in this PR, or explain why multi had to be solved at once.

Issue

closes #40

Details

copyURI

Further Enhancements

  1. Make the icon's color match the URI
  2. Fix the tooltip position (looks like a common issue,see Fix Community Usage's tooltip position #210 , and according to a quick search will need either overriding the bootstrap's tooltip or creating a one using pure CSS or a third option)

Copy link
Member

@bryan-brancotte bryan-brancotte left a comment

Choose a reason for hiding this comment

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

Hi @HagerDakroury

Great for tackling the issue, @matuskalas who will be happy to have a less un-intuitive functionality. I have remarks considering the look-and-feel, and also I want to extends this copy ability to all terms. I am adding it to the issue

@HagerDakroury HagerDakroury marked this pull request as draft June 4, 2021 11:12
@HagerDakroury
Copy link
Collaborator Author

Hi @HagerDakroury

Great for tackling the issue, @matuskalas who will be happy to have a less un-intuitive functionality. I have remarks considering the look-and-feel, and also I want to extends this copy ability to all terms. I am adding it to the issue

Converting this to a draft and moving the discussion to the issue at this moment 👍 And love to hear what @matuskalas thinks concerning the design too!

bryan-brancotte and others added 4 commits June 7, 2021 09:40
* Warping the link in a btn-group so copy functionality can be appended after

* Reducing text length of the URI when not translated
@HagerDakroury
Copy link
Collaborator Author

This is how it looks like now after adapting the design you suggested @bryan-brancotte . Would that be good to go with a clearer indication that it's a copy button?

cpyNew

@bryan-brancotte bryan-brancotte marked this pull request as ready for review June 7, 2021 12:14
Copy link
Member

@bryan-brancotte bryan-brancotte left a comment

Choose a reason for hiding this comment

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

Hi @HagerDakroury
Super great, and I was about to say something about the html ID but you also already fixed it !
A feature request : when we click to copy an element, maybe the others copy-buttons who went from fa-check to fa-checkshould go back tofa-copy`.

@HagerDakroury
Copy link
Collaborator Author

Hi @HagerDakroury
Super great, and I was about to say something about the html ID but you also already fixed it !
A feature request : when we click to copy an element, maybe the others copy-buttons who went from fa-check to fa-checkshould go back tofa-copy`.

Yeah, I actually was thinking about how to tackle this.

Do you think it should be a timeout thing? (like Github, the icon is toggled back after ~3 seconds) or whenever a new button is clicked?

@bryan-brancotte
Copy link
Member

I did not though of the timeout things, but it is way better than what I proposed initially.

@HagerDakroury
Copy link
Collaborator Author

I did not though of the timeout things, but it is way better than what I proposed initially.

Added. With a timeout of 1 sec (the Copied! tooltip is also removed after the timeout)

cpyTimeout

Copy link
Member

@bryan-brancotte bryan-brancotte left a comment

Choose a reason for hiding this comment

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

Perfect, let's squash and merge (once the possible dead code is removed/not dead code)

css/index.css Outdated Show resolved Hide resolved
@bryan-brancotte
Copy link
Member

Squash and merge whenever you fell ready for it :) 🏆

@HagerDakroury HagerDakroury merged commit 8498730 into main Jun 7, 2021
@HagerDakroury HagerDakroury deleted the uri-cpy branch June 7, 2021 14:39
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.

Add a button next to the url identifier to easily copy the actual edam url
3 participants