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

doc: adopt MDN style for kbd elements #35460

Merged
merged 1 commit into from
Oct 4, 2020
Merged

doc: adopt MDN style for kbd elements #35460

merged 1 commit into from
Oct 4, 2020

Conversation

Trott
Copy link
Member

@Trott Trott commented Oct 2, 2020

Before:

image

After:

image

The previous "After" version" After:

image

@nodejs-github-bot nodejs-github-bot added the doc Issues and PRs related to the documentations. label Oct 2, 2020
@Trott
Copy link
Member Author

Trott commented Oct 2, 2020

There is also a different style proposed by @devsnek that renders like this:

image

We'd want to tweak it a tiny bit if we go with that. The Tab key rendering overlays a bit of the line below it. And we probably would want to get rid of the margin-right on it so the + doesn't have space before it. I'd be fine to use this rendering instead of MDN if it's everyone's preference. It looks a bit chunky to me, but maybe that's good in this case? My impression from the conversation in #35400 is that people generally preferred the MDN rendering (as do I, but my preference is mild).

@Trott
Copy link
Member Author

Trott commented Oct 2, 2020

Hiding this to keep the conversation-with-myself easier to follow, as this comment is resolved below.

I tried to ascertain from https://developer.mozilla.org/en-US/docs/MDN/About if there is any licensing issue from copying a few lines of CSS from MDN. Does this 10-ish lines of CSS material warrants a Creative Commons attribution?

Any uncertainty there would be an argument for using @devsnek's style instead (or sticking with what we have and maybe adding a drop-shadow as @silverwind suggested).

@Trott
Copy link
Member Author

Trott commented Oct 2, 2020

And, for completeness, here's GitHub style that @bnb suggested as a possibility:

image

@Trott
Copy link
Member Author

Trott commented Oct 2, 2020

I tried to ascertain from https://developer.mozilla.org/en-US/docs/MDN/About if there is any licensing issue from copying a few lines of CSS from MDN. Does this 10-ish lines of CSS material warrants a Creative Commons attribution?

Any uncertainty there would be an argument for using @devsnek's style instead (or sticking with what we have and maybe adding a drop-shadow as @silverwind suggested).

Licensing issue solved. Since their code samples are public domain, I grabbed their public domain CSS for the kbd element in the first code snippet/sample at https://developer.mozilla.org/en-US/docs/Web/HTML/Element/kbd. It's a little bit different from the CSS on their website, but only a little bit. Here's the rendering:

image

@silverwind
Copy link
Contributor

silverwind commented Oct 3, 2020

#35460 (comment) looks pretty good (and unique) to me but it needs a bit less padding and a bit more border-radius. On the MDN one, I don't really like the white top-padding.

PR-URL: nodejs#35460
Reviewed-By: Daijiro Wachi <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@Trott Trott merged commit cd226c0 into nodejs:master Oct 4, 2020
@Trott
Copy link
Member Author

Trott commented Oct 4, 2020

Landed in cd226c0

@Trott Trott deleted the kbd-style branch October 4, 2020 12:35
@Trott
Copy link
Member Author

Trott commented Oct 4, 2020

I'll open a follow-on PR to deal with the padding and perhaps include elements of @devsnek's proposed style too (unless they do it first or something).

@Trott
Copy link
Member Author

Trott commented Oct 4, 2020

I'll open a follow-on PR to deal with the padding and perhaps include elements of @devsnek's proposed style too (unless they do it first or something).

Hmmm...actually, the white padding is a box shadow that is vastly more subtle when not zoomed in like the screenshot...maybe it's fine. Definitely want to get the keys more aligned with the + character though so will certainly add a vertical-align....

@danielleadams danielleadams mentioned this pull request Oct 6, 2020
danielleadams pushed a commit that referenced this pull request Oct 6, 2020
PR-URL: #35460
Reviewed-By: Daijiro Wachi <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: James M Snell <[email protected]>
joesepi pushed a commit to joesepi/node that referenced this pull request Jan 8, 2021
PR-URL: nodejs#35460
Reviewed-By: Daijiro Wachi <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
doc Issues and PRs related to the documentations.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants