On mobile, hide title if logo exists #629
Conversation
|
Deploy preview for docusaurus-preview ready! Built with commit 8ce5f23 |
There was a problem hiding this comment.
Great work! The approach is sound. Please have a look at the comments and fix accordingly.
The test plan doesn't seem to match up with the PR and issue. Did you mix up the screenshots?
It would also be great to add screenshots of tablet and desktop resolutions to ensure that there are no regressions.
Lastly, tests are failing because of the formatting. Run Prettier first 😄
lib/static/css/main.css
Outdated
| .headerTitle { | ||
| font-size: 17px; | ||
| } | ||
| .headerTitleHidden { | ||
| display: none !important; |
There was a problem hiding this comment.
headerTitleHidden is not a good name because it's only hidden on smaller widths. How about headerTitleWithLogo?
There was a problem hiding this comment.
Yes, !important is needed here.
Renamed to headerTitleWithLogo.
lib/core/nav/HeaderNav.js
Outdated
| @@ -218,6 +218,12 @@ class HeaderNav extends React.Component { | |||
| (env.translation.enabled | |||
| ? this.props.language + '/versions.html' | |||
| : 'versions.html'); | |||
| let headerClass; | |||
| if (siteConfig.headerIcon) { | |||
| headerClass="headerTitleHidden" | |||
There was a problem hiding this comment.
This is not standard JS style. Should use semicolons, single quotes and space around operators.
There was a problem hiding this comment.
Consider simplifying to:
const headerClass = siteConfig.headerIcon ? 'headerTitleHidden' : 'headerTitle';
There was a problem hiding this comment.
I figured out how to use Prettier in VSCode and am now running yarn ci-check to check this before pushing to Circle CI. 📈
Changed to ternary, good suggestion 👍
amyrlam
force-pushed
the
amy/mobile-header
branch
2 times, most recently
from
437c808
to
a700f3f
Compare
Hey, I'm not sure what you mean. The screenshots aren't mixed up...but before the I added screenshots for tablet and desktop above in the PR summary. It would be cool to add snapshot testing down the road, maybe with the task where I add some Jest tests? I'm familiar with Percy.io and it seems like Jest has the same capability. |
|
I think I mixed up IS and WAS. My bad 😓. I usually write before and after, with before coming first. It's easier to read IMO. |
|
That's a good point, I'll do WAS/IS next time. |
Motivation
Resolves: #613
Have you read the Contributing Guidelines on pull requests?
yes
Test Plan
IS with this PR:
Mobile:
Tablet (no change):
Desktop (no change):
WAS:
Mobile:
Tablet (no change):
Desktop (no change):
Related PRs
It didn't seem like a docs update was necessary?
cc @yangshun @JoelMarcey