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

✨ ENH: Update icon to use GitHub style #133

Merged
merged 3 commits into from Jul 3, 2021

Conversation

choldgraf
Copy link
Member

@choldgraf choldgraf commented Jun 22, 2021

This is an update to the SVG icon + hovering behavior that we use. Now that GitHub has its own copy button auto-added, we might be able to reduce cognitive load on users by mimicking their UX. This PR is an attempt at that!

It also adds in a background color (neutral gray) rather than trying to inherit the background from its parent element, which I think was introducing some variability.

Curious what @pradyunsg thinks

GitHub behavior:

2021-06-22_12-02-58

This PR behavior:

2021-06-27_10-54-56

It is hard to get the behavior totally consistent across themes etc, and I had a hard time getting the heights to quite line up, so please make suggestions if you think there are improvements to make there (or elsewhere).

@pradyunsg
Copy link
Member

I’m ambivalent on this. The only concerns I personally have with this change is:

  • Is this sensitive to the font size of the code block in any way?
  • How does it look on Furo in dark mode? 🙃

None the less, I think this is a good idea overall — I’m happy to add a bunch of CSS to Furo to make it look right in dark mode, if necessary.

@damianavila
Copy link

Two quick things:

  • Maybe more padding to be closer to the GH one?
  • I think the green border is actually important because somehow enforces the "success" idea at the time to copy

@choldgraf
Copy link
Member Author

Thanks for the feedback all - here's what it looks like now:

SQntp41R3u

@pradyunsg - it does scale with the rem of the div (though, it did before as well). Part of my goal in having a grey background was to make this work in a more standard way across themes...do you think there are other improvements to make there?

@pradyunsg
Copy link
Member

rem of the div (though, it did before as well)

1rem in a div is definitionally equal to 1rem in another div (regardless of the font size in those individual divs).

(Aside into CSS units: 1 rem is equal to the font size on the root HTML node (html) and is consistent throughout the document. I read rem as “root em”. Within the same document, it’s possible for 1em to be equivalent to 16px in one place and 12px in another. 1 rem however will always be a constant value throughout — say equivalent to 16px.)

All this to say: I think we should use em everywhere in this plugin, such that the button’s size is mapped to the current element’s font size, and not use rem at all. It’s not a “problem” introduced in this PR, so we can also fix this separately.

How do you feel about leaning in all the way and completely mimick GitHub’s styling for this; using the exact same colours and using the exact same padding ratios?

@choldgraf
Copy link
Member Author

You have taught me something i didn't know today 🙂

Re: totally mimicking github, IMO that is reasonable. They almost certainly have put more thought into ui ux than i have haha

@choldgraf
Copy link
Member Author

choldgraf commented Jun 23, 2021

I've updated this to use em instead of rem, though didn't notice a huge difference in the result. @pradyunsg I also added Furo to our tox dependencies, to make it a bit easier to test out w/ that theme.

I think that making the button perfectly mimic GitHub is beyond my fairly basic CSS skills, but if you know of a good pattern to follow to get this behavior, I am more than happy to have you clobber this PR if you like :-)

@choldgraf
Copy link
Member Author

Here's the latest commit - I made the background a little bit lighter by default so that it's less obtrusive when just hovering over the code cell.

2021-06-27_10-54-56

I think I am pretty happy with where it is right now. @damianavila @pradyunsg anything else you'd like to see updated or improved here? I think there are other improvements to make to get it closer to GitHub style (e.g., arrows on the tooltips) but perhaps this is a nice step forward?

@damianavila
Copy link

I am enough happy with the latest version you posted above.

@pradyunsg
Copy link
Member

This looks good to me! Now, to throw a wrench... how do you feel about making this configurable? :3

@choldgraf
Copy link
Member Author

@pradyunsg making what configurable? Also, I thought you didn't like making things configurable 😅

@pradyunsg
Copy link
Member

@pradyunsg making what configurable?

The style of the copy button -- using different svgs and general style. In case someone wants to use the current aesthetic and don't like the GitHub copy button style.

I thought you didn't like making things configurable 😅

It isn't a blanket rule. Each of these is a choice where I'd like to think we'd consider the tradeoffs and what the configuration options' effects will be. 🙃

@choldgraf
Copy link
Member Author

Yeah i agree with that sentiment - was just giving you a hard time 😉

I think making the icon configurable would be pretty reasonable. For the styling etc I'd imagine we could just document the CSS rules to use and how to overwrite them manually. Does that make sense?

Is this something you think needs to be in this PR? If not, want to open an issue so we can track it?

@pradyunsg
Copy link
Member

Is this something you think needs to be in this PR? If not, want to open an issue so we can track it?

It doesn't need to done here. Let's do it separately, and only if someone complains that they want the old style back. :)

@choldgraf
Copy link
Member Author

ok cool - that makes sense. I opened up #134 to track that one, let me know if it looks OK to you :-)

@choldgraf choldgraf changed the title Update icon to use GitHub style ✨ ENH: Update icon to use GitHub style Jul 3, 2021
@choldgraf choldgraf merged commit 22ec7cc into executablebooks:master Jul 3, 2021
@choldgraf choldgraf deleted the icon branch July 3, 2021 14:30
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.

None yet

3 participants