-
Notifications
You must be signed in to change notification settings - Fork 50.2k
[Gatsby Docs Update] Sidebar + article layout updates #10735
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
Changes from all commits
38c6a41
5bc539e
547f651
fee9ec5
cd8f984
4dc9855
89c8d69
749c27a
43b588c
fe459b3
7e7ee1c
5e4dda9
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -55,7 +55,7 @@ const Header = ({location}) => ( | |
| width: 'calc(100% / 6)', | ||
| }} | ||
| to="/"> | ||
| <img src={logoSvg} alt="React" height="20" /> | ||
| <img src={logoSvg} alt="" height="20" /> | ||
| <span | ||
| css={{ | ||
| color: colors.brand, | ||
|
|
@@ -66,7 +66,15 @@ const Header = ({location}) => ( | |
| fontSize: 16, | ||
| }, | ||
| [media.lessThan('small')]: { | ||
| visibility: 'hidden', | ||
| // Visually hidden | ||
| position: 'absolute', | ||
| overflow: 'hidden', | ||
| clip: 'rect(0 0 0 0)', | ||
| height: 1, | ||
| width: 1, | ||
| margin: -1, | ||
| padding: 0, | ||
| border: 0, | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why not
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Gotcha. This is gross. 😛 But I think your logic is reasonable. The grossness is just browser stuff. 😁
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I might look into changing the markup slightly after this is merged to avoid this if you don't oppose it. |
||
| }, | ||
| }}> | ||
| React | ||
|
|
@@ -79,10 +87,12 @@ const Header = ({location}) => ( | |
| flexDirection: 'row', | ||
| alignItems: 'stretch', | ||
| overflowX: 'auto', | ||
| WebkitOverflowScrolling: 'touch', | ||
| height: '100%', | ||
| width: '50%', | ||
| width: '60%', | ||
| [media.size('xsmall')]: { | ||
| width: 'calc(100% * 2/3)', | ||
| flexGrow: '1', | ||
| width: 'auto', | ||
| }, | ||
| [media.greaterThan('xlarge')]: { | ||
| width: 'calc(100% / 3)', | ||
|
|
@@ -115,7 +125,7 @@ const Header = ({location}) => ( | |
|
|
||
| <form | ||
| css={{ | ||
| width: 'calc(100% / 6)', | ||
| width: 'calc(100% / 8)', | ||
| display: 'flex', | ||
| flexDirection: 'row', | ||
| alignItems: 'center', | ||
|
|
@@ -129,7 +139,7 @@ const Header = ({location}) => ( | |
| width: 'calc(100% / 3)', | ||
| }, | ||
| }}> | ||
| <label htmlFor="search"> | ||
| <label htmlFor="algolia-doc-search"> | ||
| <SearchSvg /> | ||
| </label> | ||
| <div | ||
|
|
@@ -148,7 +158,9 @@ const Header = ({location}) => ( | |
| color: colors.white, | ||
| width: '100%', | ||
| fontSize: 18, | ||
| fontFamily: 'inherit', | ||
| position: 'relative', | ||
| marginLeft: 5, | ||
| ':focus': { | ||
| outline: 'none', | ||
| }, | ||
|
|
@@ -186,6 +198,7 @@ const Header = ({location}) => ( | |
| <a | ||
| css={{ | ||
| padding: '5px 10px', | ||
| marginLeft: 10, | ||
| whiteSpace: 'nowrap', | ||
| ...fonts.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.
Why'd the "alt" get removed? Accident?
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.
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 👍
Uh oh!
There was an error while loading. Please reload this page.
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 have never heard this. Interesting. So it's best to explicitly specify an empty
altattribute 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.