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 Furo side bars & improve footer styling #294

Merged
merged 7 commits into from
May 10, 2023

Conversation

Eric-Arellano
Copy link
Collaborator

Before, the left table of contents and the right page of contents would not render properly on mobile. They would not show the whole content for example. This is because they were not accounting for our top nav bar.

--

This PR also fixes the "Was this page helpful?" question not properly having lines above and below it.
Screenshot 2023-05-03 at 8 56 29 AM

--

Finally, this improves mobile to not show the project name at the top of the page. We don't render it for desktop, and don't want to for mobile.

Screenshot 2023-05-03 at 8 56 54 AM

1. Don't block Furo menus
2. Don't render project name on mobile
We need to also disable it automatically detecting the theme. I'll do that in a dedicated PR
@@ -36,6 +58,7 @@ footer .helpful-container .was-helpful-thank-you {
.helpful-container .helpful-question.no-link {
color: #8B34FC;
cursor: pointer;
text-decoration: none;
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This turns off underlining so that we match the Pytorch theme.

@Eric-Arellano Eric-Arellano changed the title Fix Furo side bars on mobile & improve footer styling [WIP] Fix Furo side bars on mobile & improve footer styling May 3, 2023
@Eric-Arellano Eric-Arellano marked this pull request as draft May 3, 2023 18:33
@Eric-Arellano
Copy link
Collaborator Author

Eric-Arellano commented May 3, 2023

While working on #293, I realized this is not working how I thought. I thought that var(--header-height) refers to the header height defined by the top-nav-bar.js web component. But it does not. The web component does not export its CSS variables globally.

Instead, var(--header-height) refers instead to the Furo value set here:

https://github.com/pradyunsg/furo/blob/e7f732ea5133d29fc6ca62af0455b16ffaab00dc/src/furo/assets/styles/variables/_spacing.scss#L6-L18

It so happens that my CSS changes to use var(--header-height) generally work well.

I suspect that a better fix is for us to update the definition of --header-height to incorporate our top nav bar's height. So, we ideally only need to define the variable in qiskit_changes.css and can remove the custom CSS rules.

Turns out, even with the above insights, we still need the same CSS rules. Only, we should not be redefining the variable --header-height. This PR now uses a unique CSS variable name.

@Eric-Arellano Eric-Arellano changed the title [WIP] Fix Furo side bars on mobile & improve footer styling Fix Furo side bars on mobile & improve footer styling May 3, 2023
@Eric-Arellano Eric-Arellano marked this pull request as ready for review May 3, 2023 19:01
Copy link
Collaborator Author

@Eric-Arellano Eric-Arellano left a comment

Choose a reason for hiding this comment

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

#295 resulted in a tweak. But now ready for review :)

Comment on lines +12 to +14
/* This value is duplicated from `top-nav-bar.js`. Its definition of the variable is not
* exposed globally. Keep in sync. */
--qiskit-top-nav-bar-height: 3.5rem;
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

See #295 for an explanation of why we take this approach.

Unlike #295, this uses a more unique variable name so that we don't override Furo's --header-height.

Comment on lines -21 to -22
display: -webkit-inline-box;
display: -ms-inline-flexbox;
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

These should not be necessary. According to https://caniuse.com/?search=inline-flex, inline-flex has been supported by all browsers since 2014.

@Eric-Arellano Eric-Arellano requested a review from coruscating May 5, 2023 16:56
@Eric-Arellano Eric-Arellano changed the title Fix Furo side bars on mobile & improve footer styling Fix Furo side bars & improve footer styling May 5, 2023
@coruscating
Copy link
Collaborator

I tested on mobile and this works as expected, but I want to note that I find these three menu icons confusing:
image

Without labels, I don't really know what each one corresponds to.

@javabster
Copy link
Collaborator

javabster commented May 5, 2023

Without labels, I don't really know what each one corresponds to.

Agreed, I think the biggest issue is the similarity between the qiskit top menu hamburger icon and the furo left side hamburger, as they are basically the same icon. Is there a way to add a label to the furo one? or replace it with text? Something like "Documentation Contents" would work, and matches how we currently do the mobile view so its less of a big change for users

@Eric-Arellano
Copy link
Collaborator Author

Is there a way to add a label to the furo one? or replace it with text? Something like "Documentation Contents" would work, and matches how we currently do the mobile view so its less of a big change for users

Thanks for the suggestions! Tracked by #300. I want to keep that fix as a dedicated PR so that we can properly discuss its design because we can choose from multiple options.

This PR is still some progress (that makes follow-up PRs easier). So it's fine to land on its own.

Copy link
Collaborator

@techtolentino techtolentino left a comment

Choose a reason for hiding this comment

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

These changes look good, thanks @Eric-Arellano

🚀

Copy link
Collaborator

@techtolentino techtolentino left a comment

Choose a reason for hiding this comment

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

These changes look good, thanks @Eric-Arellano

🚀

@Eric-Arellano Eric-Arellano merged commit e844cc5 into Qiskit:main May 10, 2023
@Eric-Arellano Eric-Arellano deleted the improve-furo branch May 10, 2023 16:10
@Eric-Arellano Eric-Arellano added this to the Furo milestone May 10, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants