[Gatsby Docs Update] Sidebar + article layout updates by joecritch · Pull Request #10735 · facebook/react

joecritch · 2017-09-18T13:20:48Z

This supersedes #10707

what are the changes?:

Adds media queries and JS to make sidebar collapse/expand from a
sticky bottom bar on small screen sizes
tweaks to colour contrast (checked using aXe) and some html attributes
centralised article layout for large desktops
created a carousel-like overflow for the marketing columns on mobile.
moved 'Edit this page' to the bottom, as part of the new centralised layout.
offset note and code blocks out of the grid, so there text lines up. (this was previously an issue with CodeMirror, but i think we're good now?)

why make these changes?

improved accessibility score
improved readability on large desktop
improved use of screen space on mobile

test plan

manual testing

**what is the change?:** - Adds media queries and JS to make sidebar collapse/expand from a sticky bottom bar on small screen sizes - Many TODOs still open! This is just an initial version. **why make this change?:** Moving closer to Joe's fantastic design. :) **test plan:** Manual testing - Flarnie will insert a gif **issue:** Various items under the `Sidebar Component` on this checklist; https://github.com/bvaughn/react/wiki/Gatsby-check-list

**what is the change?:** We skim the items in the 'defaultActiveSection' to find the currently active one, and display it's title in the sticky responsive bottom navbar. Before I had just plugged in the section title as a placeholder. **why make this change?:** It's better this way. One step closer to Joe's shiny new design. :) **test plan:** Manually tested (Flarnie will insert a screenshot) **issue:** Sidebar items on this checklist; https://github.com/bvaughn/react/wiki/Gatsby-check-list

reactjs-bot · 2017-09-18T13:24:40Z

Deploy preview ready!

Built with commit 4f1839e

https://deploy-preview-10735--reactjs.netlify.com

bvaughn

CI's failing b'c of formatting issues. 😄

Mind running yarn prettier-all (in the root of the project) to fix them up?

bvaughn · 2017-09-19T15:18:54Z

www/src/components/Container/Container.js

+        maxWidth: isFooter ? 800 : 1260,
+        paddingLeft: isFooter ? 0 : 20,
+        paddingRight: isFooter ? 0 : 20,
+      },


Maybe rather than passing in isFooter we could support additional custom CSS? (Since only one instance of Container would have the footer CSS).

Good shout, feels nicer to compose. Changed :)

bvaughn · 2017-09-19T15:19:46Z

www/src/components/LayoutHeader/Header.js

          }}
          to="/">
-          <img src={logoSvg} alt="React" height="20" />
+          <img src={logoSvg} alt="" height="20" />


Why'd the "alt" get removed? Accident?

aXe was picking it up that it was a duplicate of the title; which is apparently an anti-pattern. Empty alt tags are used for presentational img tags, so I think we're good 👍

aXe was picking it up that it was a duplicate of the title; which is apparently an anti-pattern

I have never heard this. Interesting. So it's best to explicitly specify an empty alt attribute vs none at all in this case?

Upon further consideration, I think images used as links should have alt text describing the destination of the link, not the image itself. So maybe this one should say something like, "React home page"? Oh, I see what you're saying now.

bvaughn · 2017-09-19T15:20:29Z

www/src/components/LayoutHeader/Header.js

+                width: 1,
+                margin: -1,
+                padding: 0,
+                border: 0,


Why not display: none ?

Due to the lack of alt attribute (see the other comment), we still want the site title to be read if the crawler is using mobile

Gotcha. This is gross. 😛 But I think your logic is reasonable. The grossness is just browser stuff. 😁

I might look into changing the markup slightly after this is merged to avoid this if you don't oppose it.

bvaughn · 2017-09-19T15:23:57Z

www/src/components/LayoutFooter/Footer.js

+          [media.between('large', 'xlargeSmaller')]: {
+            paddingRight: 280,
+          },
+          [media.between('xlargeSmaller', 'belowSidebarFixed')]: {


I'm not sure I care for the newly-added media constants. The API was kind of built to make sense with sizes but the new strings are kind of more arbitrary. This query particular is pretty confusing. 😁 What does it mean to be between xlargerSmaller and belowSidebarFixed?

bvaughn · 2017-09-19T15:26:41Z

www/src/theme.js

+  xlargeSmaller: {min: 1100, max: 1339},
+  belowSidebarFixed: {min: 740, max: 1559}, // Used for "between()"
+  sidebarFixed: {min: 1560, max: Infinity},
+  sidebarFixedNarrowFooter: {min: 1560, max: 2000},


I don't think these constants make sense, (paricularly xlargeSmaller 😁 ), with the way the sizes were designed. If we need these new sizes, then I think we should pick a few hard-coded media query values rather than try to make them work within the media methods.

See my latest commit. I've not removed them entirely, as I think it's good to have all width constraints in one central place, but I've made it more obvious about what they're actually doing. And was able to remove belowSidebarFixed due to the new "excludeLarge" on the between function 👍

bvaughn · 2017-09-19T15:28:22Z

www/src/components/StickyResponsiveSidebar/StickyResponsiveSidebar.js

+  render() {
+    const {defaultActiveSection, location} = this.props;
+    console.log('the defaultActiveSection is ;', defaultActiveSection);
+    console.log('this.props are ', this.props);


These probably weren't intended to be left in ^

bvaughn · 2017-09-19T15:49:42Z

www/src/templates/components/Sidebar/Section.js

    return (
      <Link
-        css={[linkCss, isItemActive(location, item) && activeLinkCss]}
+        css={[linkCss, isActive && activeLinkCss]}


This isActive change is good 👍 Thanks

bvaughn · 2017-09-19T15:51:08Z

www/src/theme.js

  darker: '#20232a', // really dark blue
  brand: '#61dafb', // electric blue
  brandLight: '#bbeffd',
+  brandOnWhite: '#10819b',


nit brandDark ?

this was actually an artefact, so now removed :)

bvaughn · 2017-09-19T15:51:19Z

www/src/theme.js

  text: '#1a1a1a', // very dark grey / black substitute
-  subtle: '#777', // light grey for text
+  subtle: '#6d6d6d', // light grey for text
+  subtleOnDark: '#999',


nit: subtleDark ?

I think subtleOnDark is nicely verbose in this instance, showing it's for use on dark backgrounds. Would you agree?

Sure that's okay 😄 Feel free to ignore "nit" comments

bvaughn · 2017-09-19T16:08:55Z

www/src/theme.js

-  between(smallKey: Size, largeKey: Size) {
-    if (SIZES[largeKey].max === Infinity) {
+  between(smallKey: Size, largeKey: Size, excludeLarge: Boolean = false) {
+    if (SIZES[largeKey].max === Infinity && !excludeLarge) {


Is this conditional correct? Or did you mean to have an || instead of an &&?

Calling between(small, large, true) would create a media query with max-width even if the max size was Infinity. When would we want that?

I think it was logically correct, but it wasn't nice to read. excludeLarge is an exception because you're not concerned about the max property. So I've refactored it — 5e4dda9

I'm still confused. Isn't a max-width of Infinity the same as no max-width at all?

Ah... I see now that you're using ${SIZES[largeKey].min} below. Hm.

bvaughn · 2017-09-19T16:55:04Z

Let's follow-up on the other PR comments later. Merging for now to help get the Netlify build working. 😄

flarnie added 4 commits September 18, 2017 16:13

Rename <StickySidebar> to <StickyResponsiveSidebar>

38c6a41

fix lint and run prettier

fee9ec5

facebook-github-bot added the CLA Signed label Sep 18, 2017

joecritch force-pushed the gatsbyDocsUpdateTypographyAndArticleLayout branch 2 times, most recently from 5729ebb to 28f8881 Compare September 18, 2017 14:28

bvaughn suggested changes Sep 18, 2017

View reviewed changes

joecritch force-pushed the gatsbyDocsUpdateTypographyAndArticleLayout branch from 28f8881 to 4f1839e Compare September 18, 2017 15:22

Typography and article layout tweaks

cd8f984

joecritch force-pushed the gatsbyDocsUpdateTypographyAndArticleLayout branch from 4f1839e to 63d9f83 Compare September 19, 2017 12:34

joecritch changed the title ~~[Gatsby Docs Update] Typography and article layout tweaks~~ [Gatsby Docs Update] Sidebar + article layout updates Sep 19, 2017

Integrated sidebar with layout tweaks

4dc9855

joecritch force-pushed the gatsbyDocsUpdateTypographyAndArticleLayout branch 2 times, most recently from 16eeca3 to 0472658 Compare September 19, 2017 14:37

Added animation and chevron icon to mobile sidebar

89c8d69

joecritch force-pushed the gatsbyDocsUpdateTypographyAndArticleLayout branch from 0472658 to 89c8d69 Compare September 19, 2017 14:38

Mathspy mentioned this pull request Sep 19, 2017

[Gatsby Docs Update] Initial version of new sidebar #10707

Closed

bvaughn reviewed Sep 19, 2017

View reviewed changes

joecritch added 2 commits September 19, 2017 16:53

Making tweakpoints easier to reason about

749c27a

Removed console.logs

43b588c

joecritch added 2 commits September 19, 2017 16:56

brandOnWhite removed, was an artefact

fe459b3

removed isFooter flag in favour of merged cssProps

7e7ee1c

bvaughn reviewed Sep 19, 2017

View reviewed changes

refactor between() media query helper

5e4dda9

bvaughn merged commit bd0f28d into facebook:gatsby Sep 19, 2017

This was referenced May 20, 2020

[Snyk] Security upgrade lighthouse from 2.0.0 to 6.0.0 cajonemsft/react#22

Open

[Snyk] Security upgrade lighthouse from 2.0.0 to 6.0.0 heshamelmasry77/react#11

Open

This was referenced Mar 23, 2022

[Snyk] Fix for 1 vulnerabilities cajonemsft/react#96

Open

[Snyk] Fix for 1 vulnerabilities heshamelmasry77/react#66

Open

Conversation

joecritch commented Sep 18, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

reactjs-bot commented Sep 18, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bvaughn left a comment

Choose a reason for hiding this comment

Uh oh!

bvaughn Sep 19, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bvaughn Sep 19, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bvaughn commented Sep 19, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

joecritch commented Sep 18, 2017 •

edited

Loading

reactjs-bot commented Sep 18, 2017 •

edited

Loading

bvaughn Sep 19, 2017 •

edited

Loading

bvaughn Sep 19, 2017 •

edited

Loading