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

feature copy results to clipboard #513

Merged
merged 9 commits into from
Sep 12, 2022
Merged

Conversation

marmoure
Copy link
Contributor

while working with eXide and running eval on some scripts i found that copying the results was not easy at all.
first i can't select it all using ctrl-A as the focus was always on the editor pane and trying to do it with the mouse proven to be a hassle.
i decided to add a copy to clipboard button which copies all the results showing in the results pane.
image

index.html.tmpl Outdated Show resolved Hide resolved
src/eXide.js Outdated Show resolved Hide resolved
Copy link
Member

@joewiz joewiz left a comment

Choose a reason for hiding this comment

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

It works great! Just a couple of suggestions in the wording. Thanks for adding tests too! I notice these appear to be failing?

@marmoure
Copy link
Contributor Author

@joewiz yes seems like CI test for copying doesn't work in this environment.
the clipboard requires special permissions and i tried to work around that
but seems like it failed.
can you please run the test locally

marmoure and others added 2 commits August 15, 2022 10:37
Co-authored-by: Joe Wicentowski <[email protected]>
Co-authored-by: Joe Wicentowski <[email protected]>
@joewiz
Copy link
Member

joewiz commented Aug 15, 2022

@marmoure I also get the same "Browser context management is not supported" error shown in the CI log when running npm run cypress.

@marmoure
Copy link
Contributor Author

after updating the test i m still seeing a failure for checking the notification which is there???!!
Navbar -- should display notification (failed)

@marmoure
Copy link
Contributor Author

marmoure commented Sep 12, 2022

@joewiz this unfortunately needs a lot of work to fix the broken tests
it Fixes #499
how should we progress?

@joewiz
Copy link
Member

joewiz commented Sep 12, 2022

@marmoure Ah, I'm not sure... Is there a way to keep the tests while marking them as "pending" or "don't run"? Perhaps the situation with browser context management will change in the future, and we could just re-enable your tests.

Re: #499, this PR is indeed quite similar, but my original idea was to allow copying on a per-item basis, but yours copies the current 10 results as a whole. I don't suppose it would be hard to add a per-item copy function? I'm happy to merge this without that, but I thought I'd ask in case it helps.

@marmoure
Copy link
Contributor Author

@joewiz can you please elaborate and how that function work When you have multiple results.

Copy link
Member

@joewiz joewiz left a comment

Choose a reason for hiding this comment

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

As discussed on today's call, we will merge this as is.

@marmoure Thank you!

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