-
Notifications
You must be signed in to change notification settings - Fork 275
feat(ui5-shellbar): branding slot introduced #11320
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
Already looks good, just a few stuff to talk about:
I know we talked to have the "primaryTitle/brandingTitle" as a slot, but now I'm rethinking, what if someone wants to have custom title later on, perhaps it could be better to "reserve" the slot for that and use a property now. We can have a call about this and decide. FYI, we decided to go with the "branding" name, 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.
This whole link case seems to be covered by the branding component, perhaps, let's also check the "old" case where a href is not provided. We may need to render an element with a button role, instead of anchor, to have accessibility compliant html strucure.
} | ||
|
||
get accLogoRole() { | ||
return this.accessibilityAttributes.logo?.role || "link"; |
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.
The logo is no longer an interactive area, role link does not make sense. Seems to me that it needs to be img or presentation (decorative). This need to be checked with accessibility spec.
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.
The value returned by this getter is assigned to the anchor element, not to the logo itself. I might rename the getter to better reflect what it's used for.
The
branding
slot has been introduced to theui5-shellbar
component.Additionally, a new component called
ui5-shellbar-branding
has been added. This component combines a logo and a title, with the ability to open links. Its primary purpose is to be used within thebranding
slot, replacing the default primary title and logo of theui5-shellbar
.The branding slot and its content take higher priority. As a result, if both the branding and the primary title + logo slot are used, only the
ui5-shellbar-branding
will be displayed.