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

Add a styling for :guilabel: inline markup. #279

Closed
wants to merge 4 commits into from

Conversation

jonathanj
Copy link
Contributor

Fixes #262.

:guilabel: is a convenient way to hint that a particular action in a user interface is available, unfortunately in the RTD theme there is no styling for it, so the markup appears as plain text.

screen shot 2016-02-11 at 11 16 30

Review on Reviewable

@agjohnson
Copy link
Collaborator

Thanks for the addition! In terms of styling, here's a few things:

  • The border box looks like it extends past the line-height, this will cause paragraph lines to overlap or look ragged.
  • There should be a bit more margin on sides of the highlight
  • I'm hesitant to force all caps, though if it's a pattern we're using on elements in the same way, I think it makes sense. We force caps on nav elements, I don't recall immediately where else we might
  • Also, the styling above implies a button element, whereas the :guilabel: could be describing any element

@jonathanj
Copy link
Contributor Author

Thanks for the feedback @agjohnson!

  • The border box was basically on the edge of line-height, I've shrunk it down so that it fits more comfortably.
  • I added a small horizontal margin. I'm not sure what variable / ratio to base this off, I used base-line-height but it seems like a letter spacing variable would be better. Is there one?
  • I don't have a strong opinion on the capitals, it was mostly a personal style preference but I think they're a little clearer (there's no letter ambiguity like there can be in lowercase) and stand out more. I don't see a pattern in the use of all caps throughout the theme, why are the section captions in the sidebar capitals, for example?

Some screenshots again, on my particular resolution these two words appear right underneath one another, so it's a good illustration for the border box size.

screen shot 2016-02-15 at 23 20 19

screen shot 2016-02-15 at 23 19 42

@Makc37
Copy link

Makc37 commented Nov 7, 2016

Reviewed 1 of 5 files at r1, 4 of 4 files at r2.
Review status: all files reviewed at latest revision, all discussions resolved, some commit checks failed.


Comments from Reviewable

@jonathanj
Copy link
Contributor Author

@Makc37 I've resolved the conflicts with master.

@ericholscher
Copy link
Member

ericholscher commented Mar 2, 2017

Seems like this can be merged with a rebase. @agjohnson any objection?

Copy link
Collaborator

@agjohnson agjohnson left a comment

Choose a reason for hiding this comment

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

Updates look good, I just a noted on the color variable used.

@@ -298,6 +298,15 @@
@extend .fa-download
&:before
margin-right: 4px
.guilabel
border: 1px solid lighten($class-color, 25%)
background: lighten($class-color, 50%)
Copy link
Collaborator

Choose a reason for hiding this comment

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

This shouldn't piggy back off of class-color and should set it's own color, or use something more appropriate than class-color.

@ericholscher
Copy link
Member

Fixed this in a new branch and merged.

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

4 participants