refactor: bslib_page_main() instead of bslib_sidebar_main()
#1059
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
* Creates `main.bslib-page-main` for the container
* Uses `.bslib-page-main` as the CSS class
* `bslib-page-main-min-{width,height}` for the Sass/CSS variables
cpsievert
approved these changes
May 29, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Follow up to #1057. I realized that the main page container that I added for
page_sidebar()andpage_navbar(sidebar=sidebar())is something we will likely want to use in other contexts, ex. inpage_navbar()without a sidebar, or inpage_fillable()or in a theoreticalpage_dashboard()with/without a sidebar.This PR updates the approach to be more generic:
main.bslib-page-mainfor the container (instead of.bslib-page-sidebar-main).bslib-page-mainas the CSS class (instead of.bslib-page-sidebar-main)bslib-page-main-min-{width,height}for the Sass/CSS variables (instead ofbslib-page-sidebar-main-min-{width,height}).The most controversial decision is switching from
<div>to<main>. I think this is the right approach because the intent is to wrap the main content area and these PRs reveal that we know which container is the page's main container. I feel strongly that we should be adding a<main>landmark. But the downside is that there should only be one<main>tag on the page at a time, meaning we're making the decision for the user.That said, it's very unlikely that a user is going to add a
<main>landmark on their own and those who know about the need are also likely to use accessibility scanning tools which will surface our<main>.