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.

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

fix(website): change header logo to avoid search bar overlapping in smaller resolution #158

Merged
merged 1 commit into from
Oct 9, 2019
Merged

Conversation

shahsank3t
Copy link
Contributor

Signed-off-by: Sanket Shah [email protected]

Issue #153

Replacing the image (logo + text) to a smaller imager (logo) when the resolution is less than or equal to 480px

Screenshot

Screenshot 2019-10-10 at 1 09 51 AM

@Michael-Grover Michael-Grover added A11Y 🔓 Accessibility related Difficulty: Starter Good First Issue :octocat: Good for newcomers hacktoberfest by DigitalOcean and DEV Type: Bug 🐛 Something isn't working labels Oct 9, 2019
Copy link
Contributor

@Michael-Grover Michael-Grover left a comment

Choose a reason for hiding this comment

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

Looks great! Can you change the breakpoint from 480px to 500px and add the alt text from the issue to the image?

Set the Alt text to "Return to docs.accordproject.org homepage"

@shahsank3t
Copy link
Contributor Author

@Michael-Grover Sure, I'll change the breakpoint but adding the alt text will again cause the overlapping issue as visible below.
Screenshot 2019-10-10 at 2 01 17 AM

I believe the default alt text "Accord Project" should be fine here.
Screenshot 2019-10-10 at 2 03 49 AM

What's your take on it?

@Michael-Grover
Copy link
Contributor

@shahsank3t sorry for the confusion, when I wrote "alt text" I meant alternative text for visually impaired people using screen readers

https://moz.com/learn/seo/alt-text

@shahsank3t
Copy link
Contributor Author

@Michael-Grover Oh, I see.
Just to understand it properly, should this alt text be reflecting for both (logo + text image & smaller logo image) or only for smaller logo image?
If for both, need to check within Docusaurus to add alt text different then the title and if only for smaller logo image, can we instead have "Accord Project - Return to docs.accordproject.org homepage" as Alt text?

@Michael-Grover
Copy link
Contributor

@shahsank3t

Ideally this alt text should be for both the smaller and larger logo, because both have the function described in the alt text. So both should have identical alt text. Neither need to say "Accord Project" before "Return to docs.accordproject.org"

@shahsank3t
Copy link
Contributor Author

@Michael-Grover @jeromesimeon
The current version of Docusaurus we are using doesn't support passing a custom alt text to the image. It refers to the title itself. (Ref: https://github.com/facebook/docusaurus/blob/master/packages/docusaurus-1.x/lib/core/nav/HeaderNav.js#L315)

If there's a plan to update Docusaurus version to the latest one, they have added supported to add custom alt text (Ref: https://github.com/facebook/docusaurus/blob/master/website/docusaurus.config.js#L61-L64)

Just a heads-up, if we decide upon updating the library, there will be plenty of changes in existing siteConfig.js to catch up with the latest ones. :)

@shahsank3t
Copy link
Contributor Author

@Michael-Grover meanwhile I have pushed the change to set the breakpoint to 500px.

@jeromesimeon
Copy link
Member

Looks great on my phone, @shahsank3t !
File

Copy link
Member

@jeromesimeon jeromesimeon left a comment

Choose a reason for hiding this comment

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

Great fix. Thanks!

@jeromesimeon
Copy link
Member

@Michael-Grover @jeromesimeon
The current version of Docusaurus we are using doesn't support passing a custom alt text to the image. It refers to the title itself. (Ref: https://github.com/facebook/docusaurus/blob/master/packages/docusaurus-1.x/lib/core/nav/HeaderNav.js#L315)

If there's a plan to update Docusaurus version to the latest one, they have added supported to add custom alt text (Ref: https://github.com/facebook/docusaurus/blob/master/website/docusaurus.config.js#L61-L64)

Just a heads-up, if we decide upon updating the library, there will be plenty of changes in existing siteConfig.js to catch up with the latest ones. :)

Thanks a lot for the detailed review on that. Would you feel like opening an issue about this? (I have some questions about the siteConfig.js and which version of Docusaurus, etc but those could probably be best in a new issue).

@Michael-Grover Michael-Grover merged commit f65e9b5 into accordproject:master Oct 9, 2019
@Michael-Grover
Copy link
Contributor

@shahsank3t thank you for helping with this issue. I've logged your contribution for Hacktoberfest 👍

@shahsank3t
Copy link
Contributor Author

@Michael-Grover @jeromesimeon - My pleasure guys. Got to learn new stuff like Docusaurus today while fixing this issue.

BTW, filed another issue to discuss updating the Docusaurus - #160

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A11Y 🔓 Accessibility related Difficulty: Starter Good First Issue :octocat: Good for newcomers hacktoberfest by DigitalOcean and DEV Type: Bug 🐛 Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants