Skip to content
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.

Sign up for GitHub

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

[Gatsby Docs Update] Sidebar + article layout updates #10735

Merged
merged 12 commits into from
Sep 19, 2017
Merged

[Gatsby Docs Update] Sidebar + article layout updates #10735

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
38c6a41
Rename <StickySidebar> to <StickyResponsiveSidebar>
flarnie Sep 14, 2017
5bc539e
[Gatsby docs update] Initial version of new sidebar
flarnie Sep 14, 2017
547f651
Show correct section title based on location
flarnie Sep 18, 2017
fee9ec5
fix lint and run prettier
flarnie Sep 18, 2017
cd8f984
Typography and article layout tweaks
joecritch Sep 18, 2017
4dc9855
Integrated sidebar with layout tweaks
joecritch Sep 19, 2017
89c8d69
Added animation and chevron icon to mobile sidebar
joecritch Sep 19, 2017
749c27a
Making tweakpoints easier to reason about
joecritch Sep 19, 2017
43b588c
Removed console.logs
joecritch Sep 19, 2017
fe459b3
brandOnWhite removed, was an artefact
joecritch Sep 19, 2017
7e7ee1c
removed isFooter flag in favour of merged cssProps
joecritch Sep 19, 2017
5e4dda9
refactor between() media query helper
joecritch Sep 19, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions www/src/components/LayoutFooter/Footer.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,10 +47,10 @@ const Footer = () => (
paddingRight: 240,
},

[media.between('large', 'xlargeSmaller')]: {
[media.between('large', 'largerSidebar')]: {
paddingRight: 280,
},
[media.between('xlargeSmaller', 'belowSidebarFixed')]: {
[media.between('largerSidebar', 'sidebarFixed', true)]: {
paddingRight: 380,
},
}}>
Expand Down
2 changes: 1 addition & 1 deletion www/src/components/StickyResponsiveSidebar/StickyResponsiveSidebar.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -108,7 +108,7 @@ class StickyResponsiveSidebar extends Component {
backgroundColor: '#f7f7f7',
},

[media.between('medium', 'belowSidebarFixed')]: {
[media.between('medium', 'sidebarFixed', true)]: {
position: 'fixed',
zIndex: 2,
height: '100%',
Expand Down
4 changes: 2 additions & 2 deletions www/src/templates/components/NavigationFooter/NavigationFooter.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,11 +35,11 @@ const NavigationFooter = ({next, prev}) => (
paddingRight: 240,
},

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

[media.between('xlargeSmaller', 'belowSidebarFixed')]: {
[media.between('largerSidebar', 'sidebarFixed', true)]: {
paddingRight: 380,
},
}}>
Expand Down
2 changes: 1 addition & 1 deletion www/src/templates/components/Sidebar/Section.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,7 +74,7 @@ const activeLinkBefore = {
left: 0,
marginTop: -3,

[media.greaterThan('xlargeSmaller')]: {
[media.greaterThan('largerSidebar')]: {
left: 15,
},
};
Expand Down
2 changes: 1 addition & 1 deletion www/src/templates/components/Sidebar/Sidebar.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,7 @@ class Sidebar extends Component {
paddingLeft: 20,
position: 'relative',

[media.greaterThan('xlargeSmaller')]: {
[media.greaterThan('largerSidebar')]: {
paddingLeft: 40,
},
}}>
Expand Down
24 changes: 15 additions & 9 deletions www/src/theme.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,29 +35,35 @@ const colors = {
black: '#000000',
};

const SIZES = {
let SIZES = {
xsmall: {min: 0, max: 599},
small: {min: 600, max: 739},
medium: {min: 740, max: 979},
large: {min: 980, max: 1279},
xlarge: {min: 1280, max: 1339},
xxlarge: {min: 1340, max: Infinity},
};

// Tweakpoints
xlargeSmaller: {min: 1100, max: 1339},
belowSidebarFixed: {min: 740, max: 1559}, // Used for "between()"
// Tweakpoints
SIZES = {
...SIZES,
largerSidebar: {min: 1100, max: SIZES.xlarge.max},
sidebarFixed: {min: 1560, max: Infinity},
sidebarFixedNarrowFooter: {min: 1560, max: 2000},
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 👍

};

type Size = $Keys<typeof SIZES>;

const media = {
between(smallKey: Size, largeKey: Size) {
if (SIZES[largeKey].max === Infinity) {
between(smallKey: Size, largeKey: Size, excludeLarge: Boolean = false) {
if (SIZES[largeKey].max === Infinity && !excludeLarge) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

return `@media (min-width: ${SIZES[smallKey].min}px)`;
} else {
return `@media (min-width: ${SIZES[smallKey].min}px) and (max-width: ${SIZES[largeKey].max}px)`;
if (excludeLarge) {
return `@media (min-width: ${SIZES[smallKey].min}px) and (max-width: ${SIZES[largeKey].min - 1}px)`;
} else {
return `@media (min-width: ${SIZES[smallKey].min}px) and (max-width: ${SIZES[largeKey].max}px)`;
}
}
},

Expand Down Expand Up @@ -145,7 +151,7 @@ const sharedStyles = {
marginLeft: 80,
},

[media.between('small', 'xlargeSmaller')]: {
[media.between('small', 'largerSidebar')]: {
flex: '0 0 200px',
marginLeft: 80,
},
Expand All @@ -154,7 +160,7 @@ const sharedStyles = {
marginLeft: 40,
},

[media.greaterThan('xlargeSmaller')]: {
[media.greaterThan('largerSidebar')]: {
flex: '0 0 300px',
},

Expand Down