Skip to content
This repository has been archived by the owner on Aug 31, 2023. It is now read-only.

feat(playground): add a button to copy rome_ir to clipboard #2604

Merged
merged 9 commits into from
May 26, 2022

Conversation

IWANABETHATGUY
Copy link
Contributor

@IWANABETHATGUY IWANABETHATGUY commented May 22, 2022

Summary

add a button to copy rome_ir to clipboard

rome_ir.mp4

opy rome_ir to clipboard

Test Plan

No

Copy link
Contributor

@MichaReiser MichaReiser left a comment

Choose a reason for hiding this comment

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

Thanks for this contribution.

What are the use cases that you see for the new button? To copy the document into a PR summary?

website/playground/package.json Outdated Show resolved Hide resolved
@IWANABETHATGUY
Copy link
Contributor Author

IWANABETHATGUY commented May 23, 2022

Thanks for this contribution.

What are the use cases that you see for the new button? To copy the document into a PR summary?

As this issue said, #2587. It would be help full when you want to debug rome_ir locally, especially when the IR is very long that the IR textarea need a scrollbar to hold it.
I have been finished a nearly finished here, https://iwanabethatguy.github.io/romeir-to-rs/.

@ematipico
Copy link
Contributor

I am not sure I understand the use case. Can't you select all the text and copy it? What's the benefit of having a button in this case?

@ematipico
Copy link
Contributor

I can see the use case and the additional feature, but I can't see adding a new dependency (the toast) to the project. It seems an overkill to me... I mean, it's nice, but I don't think it's necessary for the time being.

Copy link
Contributor

@NicholasLYang NicholasLYang left a comment

Choose a reason for hiding this comment

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

Nice! I was considering adding this too, actually for the formatted output more than the IR. Would it be possible to add this to multiple views?

website/playground/package.json Outdated Show resolved Hide resolved
website/playground/src/DesktopPlayground.tsx Outdated Show resolved Hide resolved
website/playground/src/DesktopPlayground.tsx Outdated Show resolved Hide resolved
@NicholasLYang
Copy link
Contributor

Perhaps we could do without the toast? And just show an error inline or even an old school alert

@MichaReiser
Copy link
Contributor

Perhaps we could do without the toast? And just show an error inline or even an old school alert

Does an old school title attribute work? Or does it not get triggered if it gets added after the user hovers with the mouse.

@IWANABETHATGUY
Copy link
Contributor Author

2022-05-25.1.55.20.mov

@IWANABETHATGUY
Copy link
Contributor Author

IWANABETHATGUY commented May 25, 2022

I remove toast and use this little svg icon instead, it should not have bundle size issue
.

@ematipico ematipico changed the title (website): add a button to copy rome_ir to clipboard feat(playground): add a button to copy rome_ir to clipboard May 25, 2022
website/playground/src/DesktopPlayground.tsx Outdated Show resolved Hide resolved
Co-authored-by: Emanuele Stoppa <my.burning@gmail.com>
Copy link
Contributor

@NicholasLYang NicholasLYang left a comment

Choose a reason for hiding this comment

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

Sorry to keep nitpicking, but the design is a little awkward. The check mark goes away after a few seconds, which is confusing, and leads to flickering if someone clicks the button multiple times in a row.

I'd also lean towards an clipboard-copy icon instead of a Copy to Clipboard button which takes up width and potentially covers the IR output:

Screen Shot 2022-05-25 at 6 44 04 PM

.

That said, I'd be open to merging if the flickering issue and my above comments are fixed.

website/playground/src/DesktopPlayground.tsx Outdated Show resolved Hide resolved
website/playground/src/DesktopPlayground.tsx Outdated Show resolved Hide resolved
@IWANABETHATGUY
Copy link
Contributor Author

2022-05-26.1.35.48.mov

@NicholasLYang NicholasLYang merged commit 9be13ca into rome:main May 26, 2022
@IWANABETHATGUY IWANABETHATGUY deleted the website/rome-ir-copy branch May 27, 2022 03:08
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.

4 participants