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

Make terminal commands easily copyable #122

Closed
wants to merge 1 commit into from

Conversation

stefansundin
Copy link
Contributor

@stefansundin stefansundin commented Sep 16, 2023

This makes the terminal commands on the Downloads page easily copyable with a button click.

Screenshot:

image

Windows commands have the > prompt. The prompt symbols are defined in CSS so they are not selectable. 🤩

Note that the Downloads page currently doesn't work when JavaScript is disabled! I have changes locally that fix this, but I want to submit them separately to make review easier.

If the user has blocked clipboard access then they will get an error alert and the text is selected to make it easier for them to copy it manually. I was able to test this in Firefox by setting dom.allow_cut_copy=false in about:config. If anyone can figure out a way to disable clipboard write in Chrome please write a comment below (the Clipboard site permission doesn't do it).

Screenshot 2023-09-16 at 12 01 26

The look and behavior of this is inspired by the Homebrew copy widget on this page: https://formulae.brew.sh/cask/keepassxc

Please let me know if you want any changes.

@droidmonkey
Copy link
Member

droidmonkey commented Sep 16, 2023

On the downloads page, can you also make it more obvious that the user can (and maybe should) click the "See More Options" button below the suggested download? Maybe a yellow hued color or similar styling.

image

@stefansundin
Copy link
Contributor Author

Can I change all of them to use the yellow styling from the Linux download options?

Screenshot 2023-09-16 at 12 53 21

for (const el of document.querySelectorAll('.copyable')) {
const btn = document.createElement('button');
btn.title = 'Copy to clipboard';
btn.textContent = '📋';
Copy link
Member

Choose a reason for hiding this comment

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

I prefer to use a "wireframe" type icon such as https://fonts.google.com/icons?selected=Material%20Icons%3Acontent_copy%3A

This comment applies to all the icons (X and Checkmark)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In my first attempt I was using an icon like that, but over time I have grown to prefer the icons that I use now. They feel more actionable to me.

Copy link
Member

Choose a reason for hiding this comment

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

I'll give it a look, but they seem out of place compared to the rest of our page styling.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh.. I saw your change too late. Anyway, here's what the fork awesome icons look like:

Screenshot 2023-09-16 at 13 24 13 Screenshot 2023-09-16 at 13 24 30

I removed the error icon since it didn't look that good. I don't think an icon is strictly necessary in that case since there's the alert dialog.

I can remove my last commit if you prefer the unicode characters instead.

@droidmonkey
Copy link
Member

droidmonkey commented Sep 16, 2023

Yah that styling is good, maybe we need to desaturate that color a little as it is ugly. Styling the text would be nice as well, centered/bold/etc

@droidmonkey
Copy link
Member

droidmonkey commented Sep 16, 2023

Actually looks better on my computer than your screenshot (unless you fixed the scaling is a subsequent commit):

image

for (const el of document.querySelectorAll('.copyable')) {
const btn = document.createElement('button');
btn.title = 'Copy to clipboard';
btn.textContent = '📋';
Copy link
Member

Choose a reason for hiding this comment

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

I'll give it a look, but they seem out of place compared to the rest of our page styling.

@phoerious
Copy link
Member

phoerious commented Sep 20, 2023

I don't like the borders. Too much noise and the border radius is wrong.

I'm also not sure if I'm a fan of making the more options dropdown more visible. Except for the macOS special case, it's more of an expert thing.

@droidmonkey
Copy link
Member

More visible doesn't have to mean "primary focus". As it stands right now it might as well be invisible.

@droidmonkey
Copy link
Member

I updated the border radius and background color. The copy button background darkens slightly on hover.

image

@phoerious

… a button click

Also change the header styling of collapsable sections on the Downloads page.
@phoerious
Copy link
Member

I'm still not a huge fan of having so many borders. It's a bit confusing that those are two boxes, even though the commands belong together. Looks a bit like you could use one or the other. Perhaps we can combine them into one multiline command with && and \ line breaks?

@stefansundin
Copy link
Contributor Author

Looks a bit like you could use one or the other.

For the flatpak instructions you only need to run the first command if haven't already added the remote in the proper way. As a user I prefer having each command be separately copyable.

Please let me know if there is anything I can do to help get this merged. Feel free to make changes to the branch as needed. I don't really care about the minute details as long as there is a copy button. 😄

I want to contribute another change but I want this one to be merged first since I will be doing further work on this download page, and I don't want to deal with merge conflicts if I have two PRs open. This second change will make the page usable when JavaScript is disabled, because currently it does not work at all.

Thanks.

Copy link
Contributor Author

@stefansundin stefansundin left a comment

Choose a reason for hiding this comment

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

Found two indentation issues. I'm really not used to having 4 spaces to indent.

setTimeout(() => {
icon.classList.remove('fa-check');
icon.classList.add('fa-copy');
}, 1000);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Looks like I messed up the indentation on this line.

Suggested change
}, 1000);
}, 1000);

await navigator.clipboard.writeText(textNode.textContent);
icon.classList.remove('fa-copy');
icon.classList.add('fa-check');
} catch (err) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Looks like I messed up the indentation on this line.

Suggested change
} catch (err) {
} catch (err) {

@stefansundin
Copy link
Contributor Author

If there is no further work to accept this change then I will close this PR in 7 days.

@droidmonkey
Copy link
Member

I'm all for it 🤷‍♂️

@stefansundin
Copy link
Contributor Author

Feel free to reopen this if you decide you want it after all. I just don't think there's anything else I can do to help with it. 😢

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

Successfully merging this pull request may close these issues.

None yet

3 participants