-
Notifications
You must be signed in to change notification settings - Fork 1.3k
Conversation
@mayagao, thanks for your PR! By analyzing this pull request, we identified @friedbunny to be potential reviewers. |
/cc @mapbox/ios |
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.
This theme has come a long way! I’m excited about getting it in, especially for the usability improvement of making links easier to spot.
Below are a number of comments related to the design. Most are optional – things I just noticed because I’ve never scrutinized the design to this extent before. The only piece of feedback I’d consider essential is the bit about keeping the {{module_name}}
placeholder in the header instead of hard-coding the product name.
By the way, if you need any help understanding the use or relative importance of any element in this documentation, please feel free to ask.
<p class="header-col header-col--primary"> | ||
<a class="header-link" href="{{path_to_root}}index.html"> | ||
<img style="height: 25px;" class="header-icon" src="{{path_to_root}}img/mapbox.svg" alt="{{module_name}} Docs"/> | ||
<span class='header-tag'>iOS SDK Reference</span> |
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.
These templates are used for both the iOS SDK and macOS SDK. Keep {{module_name}}
here; document.sh replaces “Mapbox Reference” with “Mapbox iOS SDK Reference” or “Mapbox macOS SDK Reference” as appropriate. realm/jazzy#411 tracks making this name configurable outside the template.
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.
emm when I use {{module_name}} it only shows mapbox, not mapbox reference
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.
It’ll show “Mapbox” if you run jazzy directly. However, if you run make idocument
or make xdocument
, document.sh will automatically correct the name to “Mapbox iOS SDK” or “Mapbox macOS SDK”. (This is how “Mapbox iOS SDK Reference” shows up in the current online documentation, for example.)
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.
You do have to append “Docs” or “Reference” after {{module_name}}
, because the script looks specifically for the phrases “Mapbox Docs” and “Mapbox Reference”.
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.
background: $color_light; | ||
padding: 0.1em 0.2em; | ||
font-weight: bold; | ||
border: 1px solid #D5D5D5; |
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.
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.
Toning it down a bit would probably be fine, but I do like that there’s more emphasis on the inline code blocks now.
@@ -488,9 +536,16 @@ pre code { | |||
} | |||
|
|||
.language { | |||
background: $color_light; |
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.
Subtly color-coding the two languages is a good idea. I’ll have to consult with you when I add AppleScript code snippets to the macOS SDK documentation. 😉 On the other hand, perhaps we should take this opportunity to add a language toggle to make this color-coding unnecessary: #7448.
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.
Let’s wait on the #7448 toggle, so that this is sure to get into v3.4.0.
@@ -2,7 +2,7 @@ | |||
<ul class="nav-groups"> | |||
{{#structure}} | |||
<li class="nav-group-name"> | |||
<a class="nav-group-name-link" href="{{path_to_root}}{{section}}.html">{{section}}</a> | |||
<a class="small-heading" href="{{path_to_root}}{{section}}.html">{{section}}</a> | |||
<ul class="nav-group-tasks"> | |||
{{#children}} | |||
<li class="nav-group-task"> |
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.
border-collapse: collapse; | ||
border-spacing: 0; | ||
overflow: auto; | ||
margin: 0 0 0.85em; | ||
} | ||
|
||
tr { | ||
&:nth-child(2n) { | ||
background-color: $table_alt_row_color; |
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.
You don’t like the zebra striping? 😄 I thought it rather nice, but I agree that it’s unnecessary given the horizontal rules.
@@ -406,10 +438,7 @@ pre code { | |||
} | |||
} | |||
|
|||
.item-container { } | |||
|
|||
.item { |
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.
show on github | ||
<img class="show-on-github-icon" src="{{{path_to_root}}}img/github.svg"/> | ||
<img style="margin-bottom: 1px;" class="show-on-github-icon" src="{{{path_to_root}}}img/github.svg"/> | ||
View Source on Github |
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.
Since this text is no longer all in lowercase, please capitalize “Github” as “GitHub”.
@@ -538,6 +599,16 @@ pre code { | |||
padding-left: 2px; | |||
} | |||
|
|||
.show-on-github a { | |||
padding: 0px 5px; | |||
background: rgba(56, 135, 190, 0.12); |
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.
This makes the “View Source on GitHub” link much more prominent than before. While I like the treatment, it may cause these links to call too much attention to themselves. Unlike with GL JS, these links only go to the declaration and documentation comment for the symbol – the exact same thing as the content right above the link, except set in a monospaced font and wrapped at 80 columns. As far as I can tell, the main use case for these links is for code contributors to easily get to the relevant header to correct typos.
th, td { | ||
padding: 6px 13px; | ||
border: 1px solid $table_border_color; | ||
border: 1px solid $keyline_color; |
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.
Since this documentation is most likely to be viewed on Retina displays, we can also use darker, half-pixel-wide borders to achieve a crisper look without sacrificing subtlety.
This PR also introduces a couple regressions in terms of responsive design, which was never great to begin with. As a stress test, here’s a before and after of the documentation at the resolution of an iPhone 5s or iPhone SE: I’m ambivalent about it, but the expandable section names don’t wrap, so they scroll quite far horizontally on a narrow screen while the +/− button wraps: |
@mayagao The new screenshot triptych is nice, thanks for updating that. If you have the original sketch/psd/whatever available, please upload that to: platform/ios/originals/screenshots.sketch If not, please go ahead and delete that file. |
Thank you for the detailed feedbacks @1ec5!
|
Im going to hold on some of the style changes and avoid making the css too complicated since we are not using base here. |
@1ec5 i changed the script to show only |
I agree with removing the redundant “Mapbox” in the header, although it looks like you still need to check in that change to document.sh. Also, we need to make sure that other instances of “Mapbox iOS SDK Docs/Reference” that don’t appear next to the Mapbox logo still say “Mapbox” in them. Perhaps we can make this easier by using some easily identifiable placeholder instead of |
ok @1ec5 just pushed |
@@ -44,4 +44,4 @@ jazzy \ | |||
--output ${OUTPUT} | |||
# https://github.com/realm/jazzy/issues/411 | |||
find ${OUTPUT} -name *.html -exec \ | |||
perl -pi -e 's/Mapbox\s+(Docs|Reference)/Mapbox iOS SDK $1/' {} \; | |||
perl -pi -e 's/BRANDLESS_DOCSET_TITLE/iOS SDK $1/' {} \; |
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.
We still need the old replacement as well because of places outside the header that end up saying “Mapbox Docs” or “Mapbox References” otherwise.
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.
ok updated
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 bow before @mayagao’s awesome ability.
thank you @1ec5 @friedbunny ! |
@mayagao This looks great! I'm so glad you took this on. Well done! 🎉 |
thank you @captainbarbosa 😳😳😳 |
Some small problems:
Sketch:
overall polish the styles a bit, make the code more distinguishable from normal text
Current status:
still work in progress
cc @friedbunny