-
Notifications
You must be signed in to change notification settings - Fork 93
feat(NcAppSidebar): add 'content' slot and 'noClose' prop #6666
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
base: main
Are you sure you want to change the base?
Conversation
54cd97c
to
9d6aba4
Compare
9d6aba4
to
f086d2e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
technically makes sense - not sure about design.
But in general I like the idea of splitting it into a layout component and the specialized components, so 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense
@@ -1155,7 +1166,7 @@ export default { | |||
* @public | |||
*/ | |||
focus() { | |||
this.$refs.header.focus() | |||
this.$refs.header?.focus() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sidebar must be focused on open, we cannot just hide the error here... I'll check tomorrow with a screen reader.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what about:
const element = tabbable(this.$refs.sidebar)[0]
if (element) {
element.focus()
}
Then it either will fall back either to:
- focusable/tabbable elements provided by developers
- close button
- <here we can throw a warning, I guess>
this.$refs.header
is either non-visible or has tabindex -1
, it should not be in focus in both cases
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For screen readers you could also focus non interactive elements to update the position of the "viewer"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
checking main branch now:
ref="toggle"
was lost in 6e13c04focus()
method has it:(this.$refs.header ?? this.$refs.toggle?.$el)?.focus()
- should be
this.$refs.toggle?.$el
, or NcButton should implement and exposefocus()
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Paddings seem to be off, also close button will not be placed consistent.
I think we need to polish the implementation a bit before pushing this to public API.
Signed-off-by: Maksim Sukharev <[email protected]>
Signed-off-by: Maksim Sukharev <[email protected]>
Signed-off-by: Maksim Sukharev <[email protected]>
ce12f05
to
8319b4e
Compare
☑️ Resolves
content
slotnoClose
prop (for custom implementation in the proper layout)🖼️ Screenshots
Quick prototype for Talk:
2025-04-23_13h59_29.mp4
🏁 Checklist
stable8
requested