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

NEW: Adding a section for footer-content like in sphinx-basic-ng #861

Merged
merged 11 commits into from
Aug 18, 2022

Conversation

AakashGfude
Copy link
Contributor

@AakashGfude AakashGfude commented Aug 8, 2022

Added footer-content.html, following the sphinx-basic-ng design.

For this and to mimic sphinx-basic-ng design (which we are ultimately planning to inherit) #840, have reordered and created a few tags for body content. Copied over the container CSS for these elements as well from sphinx-basic-ng, including the page width to be 88rem (which looked nicer to me, but can revert it.).
slack-imgs

The specific tags created/reordered here are:

  • bd-container__inner now has a new main tag and primary sidebar as its only children. secondary sidebar has been moved from this position.
  • new main tag is introduced to accommodate footer-content section. The visual position of which is shown in the pic above.
  • main tag has bd-content and the new footer-section as its children tags.
  • bd-content now has bd-article-container and the secondary sidebar as its children. Note that the secondary sidebar was earlier on the same level as primary sidebar, but has moved here to adhere to the skeleton of sphinx-basic-ng.
  • bd-article-container has an article header, article content, and article footer in it.

Inspecting the HTML, in the preview of this PR will help understand the hierarchy better.

The need for this section was highlighted in a PR in sphinx-book-theme: executablebooks/sphinx-book-theme#597

@AakashGfude AakashGfude changed the title Footer content NEW: Adding a section for footer content as in sphinx-basic-ng Aug 8, 2022
@AakashGfude AakashGfude changed the title NEW: Adding a section for footer content as in sphinx-basic-ng NEW: Adding a section for footer content like in sphinx-basic-ng Aug 9, 2022
@choldgraf
Copy link
Collaborator

@AakashGfude this looks good in general to me, though I'm a bit surprised at how many changed lines of code there are. Are those just rearrangements? Could you please:

  • In the top description, could you be more specific than just "reordered and created a few body tags"? What did you rearrange specifically? In particular anything that might be a breaking change for downstream CSS
  • Does this change the end output at all? I couldn't see any difference in our site.
  • Doesn't the theme already have a footer that spans the entire bottom of the page? How is this different from that? e.g. from our main docs:
    image

@AakashGfude AakashGfude changed the title NEW: Adding a section for footer content like in sphinx-basic-ng NEW: Adding a section for footer-content like in sphinx-basic-ng Aug 9, 2022
@AakashGfude
Copy link
Contributor Author

Thanks, @choldgraf, have updated the top description to explain the HTML container structure better.
It does not change the output at all of this theme, because we don't use footer-content at all here (Which is introduced in this PR). However, I have made the page-width to be 88rem (same as sphinx-basic-ng), while I was adjusting the styles to the new HTML layout.

The screenshot which you have posted is of footer.html which is in bright red in the snapshot of sections in the top description. And it spans the whole page.

The one introduced here is footer-content, which spans the article plus the sidebar-secondary width. The need for this section arises in sphinx-book-theme . Notice the footer in the image below:

Screen Shot 2022-08-09 at 10 43 17 pm

It will not break any downstream CSS, as the hierarchy has been matched in the SCSS files, but I will keep looking tomorrow for any unintended changes.

@choldgraf
Copy link
Collaborator

choldgraf commented Aug 10, 2022

A quick thought - could you update our layout figure at the link below to include this?

This will also make it easier to figure out what exactly we are adding

@AakashGfude
Copy link
Contributor Author

AakashGfude commented Aug 11, 2022

Good idea @choldgraf, have added the section for footer content.

Screen Shot 2022-08-11 at 11 47 03 am

Copy link
Collaborator

@choldgraf choldgraf left a comment

Choose a reason for hiding this comment

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

A few quick thoughts in there but it looks pretty good to me in general. My main questions are around the ramifications of removing the bootstrap class helper and hard-coding width instead.

I took a look at the demo docs and they all seem to look good!

.bd-page-width {
width: 100%;
@include media-breakpoint-up(lg) {
width: $breakpoint-page-width;
Copy link
Collaborator

Choose a reason for hiding this comment

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

why not just set max-width: $breakpoint-page-width instead of a fixed width?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

True, I will use max-width. I was following the sphinx-basic-ng convention https://github.com/pradyunsg/sphinx-basic-ng/blob/main/src/sphinx_basic_ng/theme/basic-ng/static/skeleton.css#L294 , but it has different fixed widths for a lot of sizes.

Copy link
Collaborator

Choose a reason for hiding this comment

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

If you think it's better to fix the width, either for our own reasons or just to be the same as the basic-ng theme, I think it's fine. We just need to encode this context in the CSS comments or variable names so it doesn't seem like a random CSS decision

Copy link
Contributor Author

Choose a reason for hiding this comment

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

flex-direction: column;
gap: 1rem;

flex-shrink: 0;
Copy link
Collaborator

Choose a reason for hiding this comment

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

what does this do?

and what was the reason for removing the flex/flex-direction/etc stuff above?

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 will revert this one. The change does not handle all cases, like, when there is an ethical ads banner. It does not shift the ethical ads banner to the bottom of the primary sidebar, when the toc links do not occupy the whole screen.

For example, it produces this:
Screen Shot 2022-08-12 at 1 15 03 pm

Instead of this:

Screen Shot 2022-08-12 at 1 15 18 pm

Thanks for pointing this change out.

@@ -16,7 +17,7 @@
}

padding: 2rem 1rem 1rem 1rem;
@include make-col(2);
width: 17rem;
Copy link
Collaborator

Choose a reason for hiding this comment

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

we should avoid hard-coding widths and things like that, so that we can standardize things more easily. What's the rationale for removing make-col? Are there any other effects? If I recall the col CSS does more than just set the width right?

Copy link
Contributor Author

@AakashGfude AakashGfude Aug 12, 2022

Choose a reason for hiding this comment

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

the make-col(2) class is setting up the width and max-width to be 16.6667%. And making it explicit that the flex-grow and flex-shrink should be zero.
I found flex-shrink 0 to be useful here, to prevent the sidebar from shrinking. And 17rem was looking okay, for the changed HTML structure. Which is also used by sphinx-basic-ng.
Good point, I will make it a variable.

@AakashGfude
Copy link
Contributor Author

@choldgraf let me know how it looks now.

Copy link
Collaborator

@choldgraf choldgraf left a comment

Choose a reason for hiding this comment

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

This looks good to me - two quick comments. +1 on merge once those are resolved.

<footer class="bd-footer-article">
{% include "sections/footer-article.html" %}
</footer>
{% endif %}
</div>
Copy link
Collaborator

Choose a reason for hiding this comment

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

What is this/div attracted to? It seems to be at a different indentation level then everything else . Is that just a quirk of my mobile browser?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

hmm maybe a quirk, because in my editor it is at the same level as "bd-content" and a child of "bd-main".

@AakashGfude
Copy link
Contributor Author

@choldgraf I don't have the permission to merge here. Do merge if you think it looks okay.

@choldgraf choldgraf merged commit 4b97fe0 into pydata:main Aug 18, 2022
@choldgraf
Copy link
Collaborator

Thanks for the enhancement @AakashGfude !

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.

2 participants