-
Notifications
You must be signed in to change notification settings - Fork 2
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.
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
[Docs #46] add megamenu #77
Conversation
add keys, adjust desktop menu breakpoint tighten up mobile nav a little netlify update readme remove localStorage flag tighten up desktop nav PR cleanup remove netlify file
4ab4e71
to
d5091cb
Compare
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.
Just a few small comments :)
src/components/Footer/index.js
Outdated
src={logo} | ||
style={{ maxHeight: '100px', paddingRight: '32px' }} | ||
/> | ||
<img src={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.
add an alt
attr
src/components/Footer/index.js
Outdated
@@ -172,11 +177,11 @@ const Footer = () => { | |||
</div> | |||
<div className="container netlify-badge"> | |||
<a target="_blank" href="https://www.netlify.com"> | |||
<img src="https://www.netlify.com/img/global/badges/netlify-color-bg.svg"/> | |||
<img src="https://www.netlify.com/img/global/badges/netlify-color-bg.svg" /> |
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.
alt
attr
src/constants.js
Outdated
@@ -0,0 +1 @@ | |||
export const SHOW_DOCS = true; |
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.
is this going to come from an env/config at some point?
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.
yup, it'll be environment variables - you'll see documentation in development but not production
ol > li:before { /* the only way to remove "." from an OL ?! */ | ||
content: counter(customlistcounter) " "; | ||
ol > li:before { | ||
/* the only way to remove "." from an OL ?! */ |
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 this can just be list-style: none
?
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.
nah, it's an ordered list, and the original developer wanted to change 1.
to 1
(no period)
@@ -0,0 +1,29 @@ | |||
$blue-bg: #6ac3e8; |
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.
thank you for colour vars!! at some point we should standardize the names across our products :)
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.
agreed! I wasn't sure what to call most of these, but wanted the vars anyways. we can standardize any time.
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.
Merge once the SHOW_DOCS
feature flag is set up!
Ticket: overture-stack/roadmap#46
Description:
requires SHOW_DOCS feature flag (not merged yet)