Repository navigation
feat(ui5-shellbar): enhance responsive behaviour for custom branding - #14015
NakataCode wants to merge 12 commits into
Conversation
|
🚀 Deployed on https://pr-14015--ui5-webcomponents-preview.netlify.app |
PetyaMarkovaBogdanova
left a comment
There was a problem hiding this comment.
Standards
Hard Violations (documented rules). 0
Smells (judgement calls)
Magic number — logoEl.offsetWidth * 2 has no named constant. STACKING_WIDTH_RATIO would reveal intent.
Primitive Obsession — keepHidden: boolean + neverHide?: boolean on ShellBarHidableItem encodes three modes with two booleans; an illegal combination (both true) is representable. A discriminated union visibilityPolicy: "normal" | "keepHidden" | "neverHide" would be cleaner.
Spec
(a) Scope Creep (not in spec) (same as above)
Stacking measurement gate — The spec says the identifier wraps first, then stacks, then overflows. The brandingTitleWouldWrap heuristic (scrollWidth > logoWidth × 2) silently skips stacking for short titles and sends them straight to overflow. The spec shows no such conditional — stacking always precedes overflow.
(b) Implemented but Wrong
brandingOverflowLabel uses textContent instead of the product title.
Spec aria-label: "[ProductIdentifier] Home". The getter reads this.branding[0]?.textContent?.trim(), which returns all slotted text across the whole element (potentially including alt-text from the logo slot in some browsers). It should target only the title text node/slot.
(c) Logo source assumes only.
brandingOverflowLogoSrc does querySelector("[slot='logo']").src. The logo slot accepts SVG elements, which have no .src. An SVG logo produces a silent empty result in the overflow popover.
Also:
-> Padding left seems incorrect
-> Too long Primary title text wraps outside the overflow, maybe it should truncate?
👋 Heads-up: dev close is in effectThanks for the contribution! This repository is currently in dev close ahead of release This PR appears to introduce public-API changes (detected by diffing the Custom Elements Manifest against the latest published version on npm):
Could you please hold off on merging into If this change must ship in the current release, please request a review from one or two members of @UI5/ui5-team-webc so the team can sign off explicitly.
Posted automatically by the Dev Close Notice workflow. |
|
Hello @NakataCode , Please fix this issue as part of the overall design: Regards, |
|
Two possible concerns: 1. Logo-only branding aria-label — spec says aria-label="Home" for logo-only; implementation returns undefined (no label at all). |
The ShellBar previously hid the product identifier as soon as space ran out, regardless of how much room was actually available. This PR replaces that approach with a fully space-aware overflow loop that keeps branding visible as long as possible, progressively hiding elements only when no other option remains.
Note
Full test coverage is not yet complete.
Overflow order
Elements hide in this sequence:
What changed
Dynamic title wrapping
Before hiding the branding identifier, the overflow loop attempts to wrap the product title to two lines in place. A
tryShrinkcallback sets a computedmax-widthon the title, forces a synchronous layout flush, then re-measures. The identifier is only moved to the overflow when two lines still do not fit. The wrap is entirely space-driven, with no fixed breakpoints.Branding in the overflow popover
When the identifier or logo is hidden, a branding entry appears at the top of the overflow popover (always first,
order: -1). The logo is acloneNodeof the actual slotted element, so<img>,<svg>, andui5-avatarlogos all work. Clicking the entry fires the brandingclickevent and closes the popover. Thearia-labelfollows the pattern"[ProductIdentifier] Home".Overflow popover sizing
The popover no longer has a fixed
max-width: 20rem. It sizes to its content: when the logo and title fit side by side, they sit on one line; when they do not, the title wraps below and the popover narrows. The floor ismin-width: 100px. An oversized logo produces a horizontal scrollbar at the bottom of the popover.Branding DOM position
The branding area
<div>moved from outsideoverflow-container-innerto inside it as the first child. This means hiding it actually reduces the measured inner width.margin-inline-end: autopushes actions to the right while keeping branding left-aligned as a natural flex child.Render stability
getOverflowSignature()hashes the measurement inputs (available width, enabled features, search state, content items, branding title).updateOverflowskips re-measuring when the signature is unchanged, preventing a borderline wrap decision from oscillating between renders and spinning the render queue.requestAnimationFramesettle loop re-measures once if the row is still overflowing after the first pass, handling cases where action button widths are not ready on first mount.visibilityPolicyrefactorReplaced two overlapping booleans on
ShellBarHidableItemwith a"normal" | "keepHidden" | "neverHide"discriminated union. AneverHideitem stays in the DOM but is still reported inhiddenItemsIds, so consumers can show a compact alternative by handling thehideevent.Content slot priority
data-hide-order="N"data-never-hidehideevents)Empty branding
An empty
<ui5-shellbar-branding>(no logo, no title text) now renders nothing: it is removed from layout, tab order, and the accessibility tree, and falls back to the legacyprimaryTitle/logoslots. Runtime content changes are detected automatically viainvalidateOnChildChange.Spec-conformance fixes
max-heightrestored to1.875rem(30px); the1.5625remfigure applies only to the overflow popover logo0.5rem--sapFontHeader6Size(was--_ui5_shellbar_menu_button_title_font_size)-webkit-line-clamp: 2instead ofwhite-space: nowrap+text-overflow: ellipsisRemoved
_isSBreakPointproperty and its sync fromShellBar.onAfterRenderingbranding-stackedhidable item,BRANDING_STACKEDpriority, and_stackedhost attribute with its CSSmax-widthon the overflow popoverTesting
Cypress tests cover:
ShellBar.cy.tsx)Warning
Full Cypress coverage for all overflow scenarios is not yet complete.