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

The "Log editor data" button should copy to the clipboard if clicked with Alt/Option #137

Merged
merged 25 commits into from
Feb 3, 2022

Conversation

oleq
Copy link
Member

@oleq oleq commented Dec 2, 2021

Feature: The "Log editor data" button should copy to the clipboard if clicked with Alt/Option. Closes #136.

Copy link
Member

@przemyslaw-zan przemyslaw-zan left a comment

Choose a reason for hiding this comment

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

Perhaps svg names could be clearer?

  • clipboard -> copy-to-clipboard
  • tick -> checkmark

As far as functionality is concerned, it seems to works as expected. However, if alt is pressed and then released without using the button, Chrome switches focus to options button, and Firefox toggles options bar, both of which is somewhat annoying - this could cause poor user experience IMO. I suppose we don't want to intercept that, since that is default key behavior on these browsers, but perhaps we could consider different key than alt? shift and ctrl do not trigger any additional behavior. Not sure about that one, maybe i'm looking too deep into this.

src/editorquickactions.css Outdated Show resolved Hide resolved
src/editorquickactions.js Outdated Show resolved Hide resolved
src/editorquickactions.js Outdated Show resolved Hide resolved
tests/inspector/editorquickactions.js Outdated Show resolved Hide resolved
tests/inspector/editorquickactions.js Outdated Show resolved Hide resolved
tests/inspector/editorquickactions.js Outdated Show resolved Hide resolved
tests/inspector/editorquickactions.js Outdated Show resolved Hide resolved
tests/inspector/editorquickactions.js Outdated Show resolved Hide resolved
tests/inspector/editorquickactions.js Outdated Show resolved Hide resolved
tests/inspector/editorquickactions.js Outdated Show resolved Hide resolved
@oleq
Copy link
Member Author

oleq commented Dec 7, 2021

Thanks for the review @przemyslaw-zan.

As far as functionality is concerned, it seems to works as expected. However, if alt is pressed and then released without using the button, Chrome switches focus to options button, and Firefox toggles options bar, both of which is somewhat annoying - this could cause poor user experience IMO.

What do you mean by "options button" and "options bar"? Is it the UI of the web browser? Because I only tested on Chrome/FF on Mac and there's nothing exceptional happening. Is it an OS thing maybe?

@oleq
Copy link
Member Author

oleq commented Dec 7, 2021

As for the changes in the code, feel free to commit them straight to the PR. They make sense :)

@przemyslaw-zan
Copy link
Member

@oleq sorry for not specifying, yes, i meant UI of the browsers, and the behavior happens on Windows

@przemyslaw-zan
Copy link
Member

przemyslaw-zan commented Dec 7, 2021

Firefox: (Options bar toggles on and off)

Chrome: (Options button takes focus and prevents further use)

To reiterate, this does not happen when alt + click is used, so it's not exactly a bug. If you think that this is negligible, or not a problem at all. you can merge.

@oleq
Copy link
Member Author

oleq commented Dec 8, 2021

I didn't anticipate that. Nice catch 👍 

I see three options now:

  • Stick with Alt (because it's alternate after all) but preventDefault the event
    • 👎 This will break native browser features that you mentioned,
    • 👎 This would also break other features in the editor like typing accents (ąęśćżźćół...),
      • OTOH... I think I already detect when the Alt key is being pressed "solo" only, so maybe this would not happen. Needs to be checked.
  • Stick with Alt but preventDefault the event only when the inspector is being hovered
    • 👍 Not breaking native browser features most of the time,
    • 👍 Not breaking accents (most of the time)
      • Or not at all, see my comment in the previous strategy.
    • 👎 Yet breaking both 👆 occasionally (accidentally), which is not a great UX. Quite a surprise when sometimes you can type things and sometimes you cannot.
    • 👎 Annoying if you move the cursor from the top of the page into inspector because the button will change the icon very late.
    • 👎 More complexity in the code.
  • Switch to Shift or Ctrl
    • 👍 Not breaking anything this time, I hope.
    • 👍 Low cost of implementation.
    • 👎 Not very intuitive because clearly, the alternate key should... alternate the features. It's a very common pattern, at least on Mac.

Which sounds best to you? I'm lazy so I'd rather go with Shift/Ctrl if confirmed safe.

@przemyslaw-zan
Copy link
Member

I agree with you on the last option being the best. Personally, I think that ctrl is the next best choice.

@oleq
Copy link
Member Author

oleq commented Dec 21, 2021

FYI: I used Shift instead of Ctrl. Turns out, Ctrl+Click is already intercepted and opens the native "right click" context menu. If I went with Ctrl, I'd have to preventDefault the 'contextmenu' event which sounds like unnecessary trouble.

Copy link
Member

@przemyslaw-zan przemyslaw-zan left a comment

Choose a reason for hiding this comment

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

LGTM

@oleq oleq merged commit 1252b78 into master Feb 3, 2022
@oleq oleq deleted the i/136 branch February 3, 2022 10:53
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.

The button that logs editor data to console should allow copying it to clipboard
3 participants