-
Notifications
You must be signed in to change notification settings - Fork 473
Add contributing options on every page #1440
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The code all looks good to me (modulo those two comments), but of course we'll need @kuanluo to weigh in on the design!
_layouts/page.html
Outdated
</button> | ||
<ul class="dropdown-menu" aria-labelledby="dropdownMenu1"> | ||
<li><a href="https://github.com/cockroachdb/docs/edit/gh-pages{{ page.url | replace: '.html', '.md'}}" target="blank">Edit This Page</a></li> | ||
<li><a href="https://github.com/cockroachdb/docs/issues/new?title={{page.title}}%20Doc%20Update%20&body=Re%3A%20%5B{{page.title}}%5D(https%3A%2F%2Fcockroachlabs.com/docs/{{page.url}})%0A%0APriority%20(Low‚%20Medium‚%20High)%3A%0A%0A%23%23%20Issue%20Description%3A%0A%0A%23%23%20Suggested%20Resolution%20&labels=&labels=community" target="blank">Report Doc Issue</a></li> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you can kill the empty &label=
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch. Done.
_layouts/page.html
Outdated
<ul class="dropdown-menu" aria-labelledby="dropdownMenu1"> | ||
<li><a href="https://github.com/cockroachdb/docs/edit/gh-pages{{ page.url | replace: '.html', '.md'}}" target="blank">Edit This Page</a></li> | ||
<li><a href="https://github.com/cockroachdb/docs/issues/new?title={{page.title}}%20Doc%20Update%20&body=Re%3A%20%5B{{page.title}}%5D(https%3A%2F%2Fcockroachlabs.com/docs/{{page.url}})%0A%0APriority%20(Low‚%20Medium‚%20High)%3A%0A%0A%23%23%20Issue%20Description%3A%0A%0A%23%23%20Suggested%20Resolution%20&labels=&labels=community" target="blank">Report Doc Issue</a></li> | ||
<li><a href="https://github.com/cockroachdb/docs/issues/new?title=New%20Doc%20Proposal%20&body=Priority%20(Low‚%20Medium‚%20High)%3A%0A%0A%23%23%20Title%0A%0A%0A%23%23%20Description%0A%0A%0A%23%23%20Outline%0A%0A%0A%23%23%20Expected%20Audience&labels=community" target="blank">Suggest New Content</a></li> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To be valid HTML, these &
s (here and the line above) should be escaped as &
, though most browsers don't really care.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, @benesch. I think I caught them all now.
TFTR, @benesch! @sploiselle, @mjibson, @spencerkimball, @petermattis, @bdarnell, if you have additional thoughts/feedback, please share. @kuanluo, can you chime in on styling? I've tried to use some other "button" conventions here, while trying to keep these options a bit less in-your-face than a real button. But the drop-down coloring should still be styled. Interested in your overall design guidance, but some specific questions:
|
LGTM |
Great idea, @jseldess! Some ideas to your questions:
I like the current placement, and we can certainly track the usage and improve from there. One idea is to repeat the button at the end of the page, next to "Was this page helpful" to capture those who read the whole way through and might have ideas/suggestions.
I prefer having the contribution options be repositioned below the title. My hypothesis is that the second option would require less work and be equally effective.
If we position the contributing option below the title, per the question above, then on mobile, it should behave the same as on desktop. Currently, it looks like the dropdown is slightly cut off on the right edge in the smaller window, but other than that, it works just fine. Another suggestion would be to place "edit this page" as the last option in the dropdown, so report and suggest are prioritized? On the design of the dropdown itself, I overall love it. Just a few nits to make sure we don't have different type spaces floating around on the site. :)
Let me know if you have any questions! |
@kuanluo, thanks for the feedback. I still need to deal with positioning, but please take another look at the styling of the menu. I've tried to address 2 & 3 above. As for 1, I can't find relevant differences to the topnav. Weight and all caps seems correct. But please let me know if I'm missing something. |
@kuanluo, figured out how to make |
LGTM, @jseldess! Thank you for taking the time to fix the styling. Much appreciated! |
So cool, @jseldess! LGTM. Have one really minor nit. Review status: 0 of 6 files reviewed at latest revision, 3 unresolved discussions. _layouts/page.html, line 14 at r1 (raw file): Previously, jseldess (Jesse Seldess) wrote…
Same thing here with the line feed _layouts/page.html, line 14 at r3 (raw file):
I'd add a line feed (
Comments from Reviewable |
_layouts/page.html, line 14 at r3 (raw file): Previously, sploiselle (Sean Loiselle) wrote…
Done. Thanks, Sean. Comments from Reviewable |
_layouts/page.html, line 14 at r1 (raw file): Previously, sploiselle (Sean Loiselle) wrote…
Done. Comments from Reviewable |
How's this, @jseldess? I force-pushed your branch, so you'll want to |
Perfect, @benesch! Thanks very much. |
- Edit This Page, which should prompt user to fork the repo - Report Doc Issue, which should open an issue with template text and community label - Suggest New Content, which should open an issue with template text and community label
My pleasure, @jseldess! |
@benesch There's nothing you can't do! Can I get a tl;dr of what you did to the |
Yeah, totally! Are you familiar with flexbox? It's a new layout model for CSS that makes it possible or at least a whole lot less painful to achieve certain layouts. This contributing button is a good example of where flexbox shines; without flexbox, I think you'd need JavaScript. Basically, you opt-in to flexbox on a per-element basis by specifying |
Oh, I forgot to mention: all that margin hacking (-30px on the container, 30px on the children) ensures that the title and the contribute button always have 30px of space from each other, but no space from the edge of the container. Adding a |
Edit This Page
, which should prompt user to fork the repoReport Doc Issue
, which should open an issue with template textand community label
Suggest New Content
, which should open an issue with template textand community label
To NOT add contributing options to a page,
contribute: false
must be set in the page's front-matter.This change is