Skip to content
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

feat: support reverse button collapsing order in menu-bar #7124

Merged
merged 5 commits into from
Feb 12, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions packages/menu-bar/src/vaadin-menu-bar-mixin.d.ts
Original file line number Diff line number Diff line change
Expand Up @@ -138,6 +138,13 @@ export declare class MenuBarMixinClass {
*/
openOnHover: boolean | null | undefined;

/**
* If true, the buttons will be collapsed into the overflow menu
* starting from the "start" end of the bar instead of the "end".
* @attr {boolean} reverse-collapse
*/
reverseCollapse: boolean | null | undefined;

/**
* Closes the current submenu.
*/
Expand Down
55 changes: 43 additions & 12 deletions packages/menu-bar/src/vaadin-menu-bar-mixin.js
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just noticed that the button border-radius values not correct with reverseCollapse. Will fix.

Screenshot 2024-02-08 at 13 32 59

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done. Couldn't find a CSS-only fix for this so I needed to compute first-visible and last-visible attributes for the buttons.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can this be considered a breaking change? I mean, if someone is customizing the buttons based on the previous CSS selectors (*-of-type), how would it behave with the new selectors?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When reverseCollapse is not used, the previous selectors should work as before.

Original file line number Diff line number Diff line change
Expand Up @@ -135,6 +135,15 @@ export const MenuBarMixin = (superClass) =>
type: Boolean,
},

/**
* If true, the buttons will be collapsed into the overflow menu
* starting from the "start" end of the bar instead of the "end".
* @attr {boolean} reverse-collapse
*/
reverseCollapse: {
type: Boolean,
},

/**
* @type {boolean}
* @protected
Expand Down Expand Up @@ -162,6 +171,7 @@ export const MenuBarMixin = (superClass) =>
'__hasOverflowChanged(_hasOverflow, _overflow)',
'__i18nChanged(i18n, _overflow)',
'_menuItemsChanged(items, _overflow, _container)',
'_reverseCollapseChanged(reverseCollapse, _overflow, _container)',
];
}

Expand Down Expand Up @@ -320,6 +330,18 @@ export const MenuBarMixin = (superClass) =>
}
}

/**
* A callback for the 'reverseCollapse' property observer.
*
* @param {boolean | null} _reverseCollapse
* @private
*/
_reverseCollapseChanged(_reverseCollapse, overflow, container) {
if (overflow && container) {
this.__detectOverflow();
}
}

/** @private */
__hasOverflowChanged(hasOverflow, overflow) {
if (overflow) {
Expand Down Expand Up @@ -410,34 +432,35 @@ export const MenuBarMixin = (superClass) =>
const isRTL = this.__isRTL;
const containerLeft = container.getBoundingClientRect().left;

let i;
for (i = buttons.length; i > 0; i--) {
const btn = buttons[i - 1];
const btnStyle = getComputedStyle(btn);
const btnLeft = btn.getBoundingClientRect().left - containerLeft;
const remaining = [...buttons];
while (remaining.length) {
const lastButton = remaining[remaining.length - 1];
const btnLeft = lastButton.getBoundingClientRect().left - containerLeft;

// If this button isn't overflowing, then the rest aren't either
if (
(!isRTL && btnLeft + btn.offsetWidth < container.offsetWidth - overflow.offsetWidth) ||
(!isRTL && btnLeft + lastButton.offsetWidth < container.offsetWidth - overflow.offsetWidth) ||
(isRTL && btnLeft >= overflow.offsetWidth)
) {
break;
}

const btn = this.reverseCollapse ? remaining.shift() : remaining.pop();

// Save width for buttons with component
btn.style.width = getComputedStyle(btn).width;
btn.disabled = true;
btn.style.visibility = 'hidden';
btn.style.position = 'absolute';
// Save width for buttons with component
btn.style.width = btnStyle.width;
}
const items = buttons.filter((_, idx) => idx >= i).map((b) => b.item);

const items = buttons.filter((b) => !remaining.includes(b)).map((b) => b.item);
this.__updateOverflow(items);

const remaining = buttons.slice(0, i);
// Ensure there is at least one button with tabindex set to 0
// so that menu-bar is not skipped when navigating with Tab
if (i > 0 && !remaining.some((btn) => btn.getAttribute('tabindex') === '0')) {
this._setTabindex(remaining[i - 1], true);
if (remaining.length && !remaining.some((btn) => btn.getAttribute('tabindex') === '0')) {
this._setTabindex(remaining[remaining.length - 1], true);
}
}
}
Expand All @@ -461,6 +484,14 @@ export const MenuBarMixin = (superClass) =>

const isSingleButton = newOverflowCount === buttons.length || (newOverflowCount === 0 && buttons.length === 1);
this.toggleAttribute('has-single-button', isSingleButton);

// Apply first/last visible attributes to the visible buttons
buttons
.filter((btn) => btn.style.visibility !== 'hidden')
.forEach((btn, index, visibleButtons) => {
btn.toggleAttribute('first-visible', index === 0);
btn.toggleAttribute('last-visible', !this._hasOverflow && index === visibleButtons.length - 1);
});
}

/** @protected */
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ snapshots["menu-bar basic"] =
`<vaadin-menu-bar role="menubar">
<vaadin-menu-bar-button
class="home"
first-visible=""
role="menuitem"
tabindex="0"
>
Expand All @@ -28,6 +29,7 @@ snapshots["menu-bar basic"] =
</vaadin-menu-bar-button>
<vaadin-menu-bar-button
class="help"
last-visible=""
role="menuitem"
tabindex="0"
>
Expand Down
44 changes: 44 additions & 0 deletions packages/menu-bar/test/overflow.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -203,6 +203,50 @@ describe('overflow', () => {
menu.i18n = { ...menu.i18n, moreOptions: '' };
expect(overflow.hasAttribute('aria-label')).to.be.false;
});

describe('reverse-collapse', () => {
beforeEach(() => {
menu.reverseCollapse = true;
});

it('should show overflow button and hide the buttons which do not fit', () => {
assertHidden(buttons[0]);
expect(buttons[0].disabled).to.be.true;
assertHidden(buttons[1]);
expect(buttons[1].disabled).to.be.true;
assertHidden(buttons[2]);
expect(buttons[2].disabled).to.be.true;
assertVisible(buttons[3]);
expect(buttons[3].disabled).to.be.false;
assertVisible(buttons[4]);
expect(buttons[4].disabled).to.be.true;

expect(overflow.hasAttribute('hidden')).to.be.false;
});

it('should set items to overflow button for buttons which do not fit', () => {
expect(overflow.item).to.be.instanceOf(Object);
expect(overflow.item.children).to.be.instanceOf(Array);
expect(overflow.item.children.length).to.equal(3);
expect(overflow.item.children[0]).to.deep.equal(menu.items[0]);
expect(overflow.item.children[1]).to.deep.equal(menu.items[1]);
expect(overflow.item.children[2]).to.deep.equal(menu.items[2]);
});

it('should update oveflow when reverseCollapse changes', () => {
menu.reverseCollapse = false;
assertVisible(buttons[0]);
expect(buttons[0].disabled).to.be.false;
assertVisible(buttons[1]);
expect(buttons[1].disabled).to.be.false;
assertHidden(buttons[2]);
expect(buttons[2].disabled).to.be.true;
assertHidden(buttons[3]);
expect(buttons[3].disabled).to.be.true;
assertHidden(buttons[4]);
expect(buttons[4].disabled).to.be.true;
});
});
});

describe('has-single-button attribute', () => {
Expand Down
10 changes: 10 additions & 0 deletions packages/menu-bar/test/visual/lumo/menu-bar.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,16 @@ describe('menu-bar', () => {
await nextRender(element);
await visualDiff(document.body, `${dir}-opened`);
});

it('reverse-collapse opened', async () => {
div.style.width = '250px';
element.reverseCollapse = true;
await nextRender(element);
element._buttons[4].click();
const overlay = element._subMenu._overlayElement;
await oneEvent(overlay, 'vaadin-overlay-open');
await visualDiff(document.body, `${dir}-reverse-collapse-opened`);
});
});

describe('single button', () => {
Expand Down
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
11 changes: 11 additions & 0 deletions packages/menu-bar/test/visual/material/menu-bar.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,17 @@ describe('menu-bar', () => {
await nextRender(element);
await visualDiff(document.body, `${dir}-opened`);
});

it('reverse-collapse opened', async () => {
div.style.width = '250px';
element.reverseCollapse = true;
element.setAttribute('theme', 'outlined');
await nextRender(element);
element._buttons[4].click();
const overlay = element._subMenu._overlayElement;
await oneEvent(overlay, 'vaadin-overlay-open');
await visualDiff(document.body, `${dir}-reverse-collapse-opened`);
});
});

describe('single button', () => {
Expand Down
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
8 changes: 4 additions & 4 deletions packages/menu-bar/theme/lumo/vaadin-menu-bar-button-styles.js
Original file line number Diff line number Diff line change
Expand Up @@ -46,14 +46,14 @@ const menuBarButton = css`
padding: 0;
}

:host(:first-of-type) {
:host([first-visible]) {
border-radius: var(--lumo-border-radius-m) 0 0 var(--lumo-border-radius-m);

/* Needed to retain the focus-ring with border-radius */
margin-left: calc(var(--lumo-space-xs) / 2);
}

:host(:nth-last-of-type(2)),
:host([last-visible]),
:host([slot='overflow']) {
border-radius: 0 var(--lumo-border-radius-m) var(--lumo-border-radius-m) 0;
}
Expand Down Expand Up @@ -86,12 +86,12 @@ const menuBarButton = css`
border-radius: 0;
}

:host([dir='rtl']:first-of-type) {
:host([dir='rtl'][first-visible]) {
border-radius: 0 var(--lumo-border-radius-m) var(--lumo-border-radius-m) 0;
margin-right: calc(var(--lumo-space-xs) / 2);
}

:host([dir='rtl']:nth-last-of-type(2)),
:host([dir='rtl'][last-visible]),
:host([dir='rtl'][slot='overflow']) {
border-radius: var(--lumo-border-radius-m) 0 0 var(--lumo-border-radius-m);
}
Expand Down
2 changes: 1 addition & 1 deletion packages/menu-bar/theme/lumo/vaadin-menu-bar-styles.js
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ registerStyles(
border-radius: var(--lumo-border-radius-m);
}

:host([theme~='end-aligned']) ::slotted(vaadin-menu-bar-button:first-of-type),
:host([theme~='end-aligned']) ::slotted(vaadin-menu-bar-button[first-visible]),
:host([theme~='end-aligned'][has-single-button]) ::slotted(vaadin-menu-bar-button) {
margin-inline-start: auto;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -49,11 +49,11 @@ const menuBarButton = css`
margin-right: 1px;
}

:host(:first-of-type) {
:host([first-visible]) {
border-radius: 4px 0 0 4px;
}

:host(:nth-last-of-type(2)),
:host([last-visible]),
:host([slot='overflow']) {
border-radius: 0 4px 4px 0;
}
Expand All @@ -72,7 +72,7 @@ const menuBarButton = css`
margin-right: -1px;
}

:host([theme~='outlined']:not([dir='rtl']):nth-last-of-type(2)),
:host([theme~='outlined']:not([dir='rtl'])[last-visible]),
:host([theme~='outlined']:not([dir='rtl'])[slot='overflow']) {
margin-right: 0;
}
Expand All @@ -83,11 +83,11 @@ const menuBarButton = css`
}

/* RTL styles */
:host([dir='rtl']:first-of-type) {
:host([dir='rtl'][first-visible]) {
border-radius: 0 4px 4px 0;
}

:host([dir='rtl']:nth-last-of-type(2)),
:host([dir='rtl'][last-visible]),
:host([dir='rtl'][slot='overflow']) {
border-radius: 4px 0 0 4px;
}
Expand All @@ -100,7 +100,7 @@ const menuBarButton = css`
margin-left: -1px;
}

:host([theme~='outlined'][dir='rtl']:nth-last-of-type(2)),
:host([theme~='outlined'][dir='rtl'][last-visible]),
:host([theme~='outlined'][dir='rtl'][slot='overflow']) {
margin-left: 0;
}
Expand Down
2 changes: 1 addition & 1 deletion packages/menu-bar/theme/material/vaadin-menu-bar-styles.js
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ registerStyles(
border-radius: 4px;
}

:host([theme~='end-aligned']) ::slotted(vaadin-menu-bar-button:first-of-type),
:host([theme~='end-aligned']) ::slotted(vaadin-menu-bar-button[first-visible]),
:host([theme~='end-aligned'][has-single-button]) ::slotted(vaadin-menu-bar-button) {
margin-inline-start: auto;
}
Expand Down
Loading