feat(page): add context selector container in sidebar and add examples#6839
feat(page): add context selector container in sidebar and add examples#6839mcoker merged 2 commits intopatternfly:v6from
Conversation
|
Preview: https://patternfly-pr-6839.surge.sh A11y report: https://patternfly-pr-6839-a11y.surge.sh |
There was a problem hiding this comment.
Tests haven't finished running, but I suspect you'll get some a11y errors from the dupe page-template--id values.
|
|
||
| ### Context selector in sidebar | ||
| ```hbs isFullscreen | ||
| {{> page-template page-template--id="nav-basic-example" page-template-sidebar--hasContextSelector='true'}} |
There was a problem hiding this comment.
| {{> page-template page-template--id="nav-basic-example" page-template-sidebar--hasContextSelector='true'}} | |
| {{> page-template page-template--id="context-selector-sidebar-example" page-template-sidebar--hasContextSelector='true'}} |
|
|
||
| ### Context selector expanded in sidebar | ||
| ```hbs isFullscreen | ||
| {{> page-template page-template--id="nav-basic-example" page-template-sidebar--hasContextSelector='true' page-template-sidebar--hasContextSelectorExpanded='true'}} |
There was a problem hiding this comment.
| {{> page-template page-template--id="nav-basic-example" page-template-sidebar--hasContextSelector='true' page-template-sidebar--hasContextSelectorExpanded='true'}} | |
| {{> page-template page-template--id="context-selector-sidebar-expanded-example" page-template-sidebar--hasContextSelector='true' page-template-sidebar--hasContextSelectorExpanded='true'}} |
| position: absolute; | ||
| left: var(--pf-v6-c-page__sidebar-body--m-context-selector--PaddingInlineStart); | ||
| right: var(--pf-v6-c-page__sidebar-body--m-context-selector--PaddingInlineEnd); | ||
| z-index: var(--pf-t--global--z-index--sm); |
There was a problem hiding this comment.
Gonna leave some nit comments, not necessary for merging or anything. I might add #ws-html-demos-c-page-context-selector-expanded-in-sidebar .pf-v6-c-page__sidebar-body.pf-m-context-selector { position: relative; } then update this and add a top: 100% so it's positioned from the sidebar-body element. Right now I think it's positioned via the parent .pf-c-sidebar element.
|
|
||
| ### Context selector in sidebar | ||
| ```hbs isFullscreen | ||
| {{> page-template page-template--id="nav-basic-example" page-template-sidebar--hasContextSelector='true'}} |
There was a problem hiding this comment.
Our partial params (always I think?) start with a capital letter. And if it's just a boolean, I think we're moving to using true and false instead of a string for "true"
| {{> page-template page-template--id="nav-basic-example" page-template-sidebar--hasContextSelector='true'}} | |
| {{> page-template page-template--id="nav-basic-example" page-template-sidebar--HasContextSelector=true}} |
|
|
||
| ### Context selector expanded in sidebar | ||
| ```hbs isFullscreen | ||
| {{> page-template page-template--id="nav-basic-example" page-template-sidebar--hasContextSelector='true' page-template-sidebar--hasContextSelectorExpanded='true'}} |
There was a problem hiding this comment.
I would probably use page-template-context-selector--IsExpanded instead of page-template-sidebar--hasContextSelectorExpanded. Or update page-template-sidebar--hasContextSelectorExpanded to page-template-sidebar--IsContextSelectorExpanded, but I think it makes more sense as page-template-context-selector--IsExpanded
| {{> page-template page-template--id="nav-basic-example" page-template-sidebar--hasContextSelector='true' page-template-sidebar--hasContextSelectorExpanded='true'}} | |
| {{> page-template page-template--id="nav-basic-example" page-template-sidebar--hasContextSelector='true' page-template-context-selector--IsExpanded=true}} |
| {{#> menu-toggle menu-toggle--modifier="pf-m-full-width" menu-toggle--IsExpanded=page-template-sidebar--hasContextSelectorExpanded}} | ||
| {{#> menu-toggle-icon}} | ||
| <i class="fas fa-fw fa-cogs" aria-hidden="true"></i> | ||
| {{/menu-toggle-icon}} | ||
| {{#> menu-toggle-text}} | ||
| Administrator | ||
| {{/menu-toggle-text}} | ||
| {{#> menu-toggle-controls}} | ||
| {{> menu-toggle-toggle-icon}} | ||
| {{/menu-toggle-controls}} | ||
| {{/menu-toggle}} |
There was a problem hiding this comment.
Just because it's usually easier for maintenance, I'd use this here. And I might also add a param to menu-toggle.hbs for menu-toggle--IsFullWidth so we can use that in place of menu-toggle--modifier="pf-m-full-width"
| {{#> menu-toggle menu-toggle--modifier="pf-m-full-width" menu-toggle--IsExpanded=page-template-sidebar--hasContextSelectorExpanded}} | |
| {{#> menu-toggle-icon}} | |
| <i class="fas fa-fw fa-cogs" aria-hidden="true"></i> | |
| {{/menu-toggle-icon}} | |
| {{#> menu-toggle-text}} | |
| Administrator | |
| {{/menu-toggle-text}} | |
| {{#> menu-toggle-controls}} | |
| {{> menu-toggle-toggle-icon}} | |
| {{/menu-toggle-controls}} | |
| {{/menu-toggle}} | |
| {{> menu-toggle | |
| menu-toggle--modifier="pf-m-full-width" | |
| menu-toggle--IsExpanded=page-template-sidebar--hasContextSelectorExpanded | |
| menu-toggle--text="Administrator" | |
| menu-toggle--icon="cogs fa-fw"}} |
There was a problem hiding this comment.
Nevermind re: the dupe page-template--id's failing the build. The build passed, we can update those or any of my other comments after beta if you'd prefer 👍
There was a problem hiding this comment.
Looks fantabulous.
|
🎉 This PR is included in version 6.0.0-alpha.177 🎉 The release is available on: Your semantic-release bot 📦🚀 |
This adds an example for a context selector in the page sidebar, and another with it expanded.
It adds a container in the sidebar modified to hold a context selector.
Fixes #5776