-
Notifications
You must be signed in to change notification settings - Fork 394
fix: Make site more mobile friendly #265
Conversation
josephperrott
commented
Sep 12, 2017
- Make navbar "sticky" on all pages
- Show table of contents (non-sticky) at the top of pages on mobile
- Shrink landing page spacing on mobile to increase density
- Move navbar to a two row version on mobile
- Use md-icon for angular and github icons in navbar
- Enforce word breaks for p tags in docs, preventing x axis overflows
- Decrease height of Guides subheader to match other pages
- Shrink docs-api table spacing on mobile to increase density
src/app/shared/navbar/navbar.html
Outdated
<img class="docs-angular-logo" | ||
src="../../../assets/img/homepage/angular-white-transparent.svg" | ||
alt="angular"> | ||
<md-icon class="angular-logo" svgIcon="angular"></md-icon> |
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.
Any reason to make this change? Switch to md-icon
does incur a cost, and we don't need to have an SVG here since we don't style the Angular logo
(same for github logo)
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 was looking to use md-icon-button
for the github icon on mobile, and while it doesn't require that a md-icon
is actually the ContentChild
, I wanted to do it "correctly". Then I figured it made sense to do it for both rather than just one of them.
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 it should be okay to keep using img
; I want to get better about encouraging people to only use md-icon
when there'd be some advantage in doing so
src/app/material-docs-app.scss
Outdated
@@ -12,6 +12,10 @@ material-docs-app > app-component-sidenav { | |||
flex: 1 1 auto; | |||
} | |||
|
|||
material-docs-app > :last-child { |
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.
Add a comment that describes what you're targeting here?
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.
Done
src/app/shared/navbar/navbar.scss
Outdated
.docs-navbar { | ||
display: none; | ||
|
||
.docs-nav-link { |
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.
Can this rule be un-nested?
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.
Done
src/app/shared/navbar/navbar.scss
Outdated
} | ||
} | ||
|
||
.show-small { |
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.
Missing prefix (here and elsewhere). The prefix is typically something like docs-navbar-
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.
Done
// Prevent p tags from not breaking, causing x axis overflows. | ||
.docs-api > p { | ||
word-break: break-word; | ||
} |
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.
Why direct child?
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 did direct child because it is the case we are seeing in the issue with. My concern with doing all indirect matching was messing up how the code formatted boxes handled word and line breaks.
@@ -1,2 +1,2 @@ | |||
<app-navbar [class.mat-elevation-z6]="showShadow"></app-navbar> | |||
<app-navbar class="mat-elevation-z6"></app-navbar> |
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.
Any way to only make the elevation show once the page has been scrolled and stickiness has taken effect? If not, I think showShadow
can be refactored out of the component class.
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 don't believe its worth the effort to make the elevation only when scrolled, especially since we are faking the stickiness of the header. I have removed showShadow
as you mentioned.
Also please consider #236! The code sample wrapping makes it really hard to read on mobile! |
d4ffc28
to
dc80642
Compare
Looks like a unit test is failing |
dc80642
to
87a699c
Compare
- Make navbar "sticky" on all pages - Show table of contents (non-sticky) at the top of pages on mobile - Shrink landing page spacing on mobile to increase density - Move navbar to a two row version on mobile - Use md-icon for angular and github icons in navbar - Enforce word breaks for p tags in docs, preventing x axis overflows - Decrease height of Guides subheader to match other pages - Shrink docs-api table spacing on mobile to increase density
87a699c
to
0e0685a
Compare
@jelbourn unit test issue resolved |