Skip to content
This repository has been archived by the owner on Jul 3, 2024. It is now read-only.

new copy button #24

Open
wants to merge 8 commits into
base: master
Choose a base branch
from
Open

new copy button #24

wants to merge 8 commits into from

Conversation

adamalston
Copy link
Member

@adamalston adamalston commented Aug 31, 2020

Fixes #12

Overview

  • Make copy button show up if there is text in the revised TextArea
  • Implement functionality to copy text from the TextArea

Screenshot_2020-09-07 Projeto Libras

Any feedback and/or edits are appreciated

@filipecorrea
Copy link
Contributor

Hi @adamalston. Thanks for your contribution. It's not clear to me why you did not follow the layout and added a new button on this screen. Do you think there is a problem with the proposed layout?

@adamalston
Copy link
Member Author

The issue said:

Add a ghost button in the Review box so that the user can copy what has been translated to the clipboard.

I followed this and put the button in the review box. Also, the issue didn't specify whether the old button should be removed. Any feedback or edits were expected since the instructions were a little unclear.

@filipecorrea
Copy link
Contributor

You're right, the issue wasn't clear enough. Sorry about that.

I just updated the issue description to the appropriate behavior that's being expected. I hope you don't mind updating your code to reflect that.

@camilasb
Copy link

camilasb commented Sep 7, 2020

Can I see how it's like somehow to check out the design?

Copy link
Contributor

@filipecorrea filipecorrea left a comment

Choose a reason for hiding this comment

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

It's working as expected, but I'm missing unit tests. I think it would be good to check if (and when) the button is rendered and validate what's happening when it's clicked.

@filipecorrea
Copy link
Contributor

@camilasb Adam updated the screenshot in the pull request description, but I'm sending another one anyway:

Screen Shot 2020-09-08 at 17 02 58

While testing this one, I found two SCSS problems unrelated to this issue: #25 and #26.

@adamalston
Copy link
Member Author

I do not have much experience testing buttons and onClick events so any assistance there would be appreciated. Further, if someone wants to contribute the tests themselves, I would fine with that.

I can confirm #25 on my end as well (macOS, Firefox 80.0.1). My screenshot appears different because I am using a dark mode extension which automatically makes the page darker and increases contrast between text and background.

@filipecorrea
Copy link
Contributor

@adamalston React Test Utilities is a good place to start and this example has all that you need to create a src/content/UserPage/UserPage.test.js.

Please give a try and I can continue helping you using code reviews.

@camilasb
Copy link

camilasb commented Sep 9, 2020

@filipecorrea if there's an env for testing, let me know :D i can take a look at that as well

@filipecorrea
Copy link
Contributor

@camilasb We don't have this code deployed yet. You'll need to run it locally at this moment.

@camilasb
Copy link

@camilasb We don't have this code deployed yet. You'll need to run it locally at this moment.

i see! tks :D

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Implement copy to clipboard button
3 participants