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

Copy dev profile #337

Merged
merged 15 commits into from Apr 22, 2022
Merged

Copy dev profile #337

merged 15 commits into from Apr 22, 2022

Conversation

gathuku
Copy link
Contributor

@gathuku gathuku commented Mar 19, 2022

Pull request checklist

  • Your code contains tests relevant for code you modified
  • You have linted and tested the project with bin/check

TODO

Credit to thoughtbot

@gathuku
Copy link
Contributor Author

gathuku commented Mar 19, 2022

@joemasilotti This will append many flash messages if you click many times, do you like the behavior?
Looking forward to using turbo stream update/replace

@joemasilotti
Copy link
Owner

Hmm. I'm wondering if changing the copy icon to a check when clicking is a better approach. We could even clear it after a few seconds. Tailwind UI does this and it works quite well.

@gathuku
Copy link
Contributor Author

gathuku commented Mar 21, 2022

That will make it simpler, do you have an example from the tailwind UI?

@joemasilotti
Copy link
Owner

If you visit any Tailwind UI example there's a clipboard icon to the right to copy the code.

Default:
image

Hover:
image

Clicked:
image


I don't think we need the tooltip, though. Instead, let's swap out the copy icon with a check (Heroics check) on click. And change it back after 2 seconds.

@gathuku
Copy link
Contributor Author

gathuku commented Mar 26, 2022

I have changed this to , do you have some ideas how this can be tested?
Screen Shot 2022-03-26 at 10 11 44 PM

when clicked
Screen Shot 2022-03-26 at 10 12 28 PM

@joemasilotti
Copy link
Owner

If you set a title in the SVG you can query for it with a CSS selector. There's an example somewhere in the codebase.

As for copy and clipboard, I'm not 100% sure. Might have to get creative if there isn't something built in to capybara.

app/components/developer_primary_action_component.html.erb Outdated Show resolved Hide resolved
app/components/developer_primary_action_component.html.erb Outdated Show resolved Hide resolved
<%= inline_svg_tag "icons/solid/document-duplicate.svg", class: "h-6 w-5", data: {clipboard_target: "toggleable"} %>
<%= inline_svg_tag "icons/solid/check.svg", class: "h-6 w-5 text-green-500 hidden", data: {clipboard_target: "toggleable"} %>
Copy link
Owner

Choose a reason for hiding this comment

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

I'd love to get rid of the mismatched height/width calls, if possible. Because I'd be inclined to change them both to *-5 but that would break the layout!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What about getting rid with w-5 and remain with height?

Copy link
Owner

Choose a reason for hiding this comment

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

I think the question is more "Why does `w-5 h-5 not work here" when it works everywhere else. Are we missing a flex tag or something?

static targets = ["toggleable"];

copy({ target: { value } }) {
navigator.clipboard.writeText(value);
Copy link
Owner

Choose a reason for hiding this comment

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

It seems like this is copying plain text and not text/html. This SO question has a few answers that could help. The accepted on, even though it is a lot of code, does indeed work for me. You can test by pasting the copied code into an email client.

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 don't know how trello works but if you paste a link https://railsdevs.com/developers/35
It's able to transform to
Screen Shot 2022-04-07 at 6 01 21 PM

Copy link
Owner

Choose a reason for hiding this comment

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

That's only the link though. Trello is fetching the title by crawling the page and populating it from <title>.

We want full HTML to be copied, so the text with an embedded link. Not only a link by itself.

app/javascript/controllers/index.js Outdated Show resolved Hide resolved
@@ -8,4 +8,11 @@
<%= inline_svg_tag "icons/solid/briefcase.svg", class: "-ml-1 mr-3 h-5 w-5" %>
<%= t("developers.show.hire") %>
<% end %>

<% if admin? %>
<button data-controller="clipboard" data-action="click->clipboard#copy click->clipboard#toggle" value="<a href='<%= developer_url(@developer) %>'><%= @developer.hero %></a>" type="button" class="shrink-0 mt-4 md:mt-0 inline-flex items-center px-4 py-2 border border-gray-300 shadow-sm text-base font-medium rounded-md text-gray-700 bg-white hover:bg-gray-50 focus:outline-none focus:ring-2 focus:ring-offset-2 focus:ring-gray-500">
Copy link
Owner

Choose a reason for hiding this comment

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

Is shoving this in the value the best approach? I wonder if it would be better to move this to a hidden element inside the controller with a target. But read my comment below on text/html first because that might help guide you a bit.

@joemasilotti
Copy link
Owner

Hey @gathuku, do you still have the appetite to get this across the finish line? I made a lot of comments in the PR review. If not, let me know and I can take over.

@gathuku
Copy link
Contributor Author

gathuku commented Apr 7, 2022

Sure, Am willing to look at it, i might want your help on testing.

@joemasilotti
Copy link
Owner

Sure thing! Let me know when you want to hand it off and I will take it from there.

@gathuku
Copy link
Contributor Author

gathuku commented Apr 7, 2022

Tried something like this with no success

  test "should show copy icon to admin user" do
    user = users(:admin)
    render_inline DeveloperPrimaryActionComponent.new(user:, developer: @developer)

    assert_selector "svg[title='document duplicate']"
end

@@ -0,0 +1,4 @@
<svg xmlns="http://www.w3.org/2000/svg" class="h-5 w-5" viewBox="0 0 20 20" fill="currentColor">
Copy link
Owner

Choose a reason for hiding this comment

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

@gathuku, you have to add the title attribute here for the query to work.

@joemasilotti
Copy link
Owner

Hey @gathuku, are you still working on this? I use a workaround every day that could be sped up a ton by this PR being merged so I'd love to see it get deployed soon. Let me know if you'd rather pass it off and I can take it from here!

@gathuku
Copy link
Contributor Author

gathuku commented Apr 22, 2022

Hey @gathuku, are you still working on this? I use a workaround every day that could be sped up a ton by this PR being merged so I'd love to see it get deployed soon. Let me know if you'd rather pass it off and I can take it from here!

It's okay @joemasilotti . Please pick it, i will follow how you implement this.
Thank you.

@joemasilotti joemasilotti merged commit 67b5438 into joemasilotti:main Apr 22, 2022
5 checks passed
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.

Button to copy hero/URL of developer profile
2 participants