Skip to content

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

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

Antreesy
Copy link
Contributor

@Antreesy Antreesy commented Mar 28, 2025

☑️ Resolves

🖼️ Screenshots

Quick prototype for Talk:

Clean WIth explanation
image image
2025-04-23_13h59_29.mp4

🏁 Checklist

  • ⛑️ Tests are included or are not applicable
  • 📘 Component documentation has been extended, updated or is not applicable
  • 3️⃣ Backport to stable8 requested

@Antreesy Antreesy added enhancement New feature or request 3. to review Waiting for reviews feature: app-sidebar Related to the app-sidebar component labels Mar 28, 2025
@Antreesy Antreesy added this to the 8.24.0 milestone Mar 28, 2025
@Antreesy Antreesy self-assigned this Mar 28, 2025
@Antreesy Antreesy force-pushed the feat/noid/appsidebar-header-empty-slot branch 2 times, most recently from 54cd97c to 9d6aba4 Compare March 31, 2025 07:50
@Antreesy Antreesy changed the title feat(NcAppSidebar): provide 'header-empty' slot for customization feat(NcAppSidebar): add 'content' slot and 'noClose' prop Mar 31, 2025
@Antreesy Antreesy mentioned this pull request Mar 31, 2025
@Antreesy Antreesy force-pushed the feat/noid/appsidebar-header-empty-slot branch from 9d6aba4 to f086d2e Compare March 31, 2025 09:06
Copy link
Contributor

@susnux susnux left a 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 👍

@Antreesy Antreesy requested a review from ShGKme March 31, 2025 17:56
Copy link
Contributor

@DorraJaouad DorraJaouad left a 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()
Copy link
Contributor

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.

Copy link
Contributor Author

@Antreesy Antreesy Mar 31, 2025

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

Copy link
Contributor

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"

Copy link
Contributor Author

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 6e13c04
  • focus() method has it: (this.$refs.header ?? this.$refs.toggle?.$el)?.focus()
  • should be this.$refs.toggle?.$el, or NcButton should implement and expose focus()

Copy link
Contributor

@susnux susnux left a 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.

@ShGKme ShGKme modified the milestones: 8.24.0, 8.25.0 Apr 2, 2025
@Antreesy Antreesy force-pushed the feat/noid/appsidebar-header-empty-slot branch from ce12f05 to 8319b4e Compare April 8, 2025 13:27
@Antreesy Antreesy changed the base branch from stable8 to main April 8, 2025 13:27
@Antreesy
Copy link
Contributor Author

Antreesy commented Apr 8, 2025

Added --app-sidebar-close-button-offset variable exposed to <header class="app-sidebar-header"> descendants:

noClose - true noClose - false
image image

The rest of paddings are set by default header content, and should not be enforced to custom content (like with background-image, there might be a need to fill all the space available)

As all of the new stuff, moved to main branch to settle it here first.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3. to review Waiting for reviews enhancement New feature or request feature: app-sidebar Related to the app-sidebar component
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants