Skip to content

fix(ui5-toolbar): fix overflow layout processing in dialogs #11894

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

Merged
merged 9 commits into from
Jul 17, 2025

Conversation

plamenivanov91
Copy link
Contributor

@plamenivanov91 plamenivanov91 commented Jul 10, 2025

There was a problem with the ui5-toolbar component when used inside a dialog. The toolbar's overflow layout was not being processed correctly after the dialog was opened, leading to issues with item visibility and layout.

This tracks for a similar scenario, which triggers the processOverflowLayout method of the toolbar. This ensures that the toolbar's layout is recalculated correctly after the dialog is opened, allowing the overflow items to be displayed properly.

Fixes: #11771

There was a problem with the `ui5-toolbar` component when used inside a dialog.
The toolbar's overflow layout was not being processed correctly after the dialog was opened,
leading to issues with item visibility and layout.

This commit adds an event listener for the `after-open` event of the dialog,
which triggers the `processOverflowLayout` method of the toolbar.
This ensures that the toolbar's layout is recalculated correctly after the dialog is opened,
allowing the overflow items to be displayed properly.

Fixes: SAP#11771
Copy link
Member

@kgogov kgogov left a comment

Choose a reason for hiding this comment

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

Hi Plamen,

Nice approach tracking the transition from hidden to visible! 👍

I tested this with a toolbar inside a dialog, but unfortunately it's not working as expected for me either. The overflow calculation still doesn't seem to trigger properly when the dialog opens.

Also we should definitely add a test case for this scenario:

it("should handle overflow correctly when toolbar becomes visible from hidden state", async () => {
    // Test toolbar inside a closed dialog that gets opened
    // Verify overflow button and item distribution work correctly
});

Image

@plamenivanov91
Copy link
Contributor Author

Hi Plamen,

Nice approach tracking the transition from hidden to visible! 👍

I tested this with a toolbar inside a dialog, but unfortunately it's not working as expected for me either. The overflow calculation still doesn't seem to trigger properly when the dialog opens.

Also we should definitely add a test case for this scenario:

it("should handle overflow correctly when toolbar becomes visible from hidden state", async () => {
    // Test toolbar inside a closed dialog that gets opened
    // Verify overflow button and item distribution work correctly
});

Image

I changed my approach for fixing the issue.

Could you try the fix?

I tried numerous ways to write a test for our case but it proves almost impossible to hit this exact case in test environment.

May you could suggest a test ?

@plamenivanov91 plamenivanov91 requested a review from kgogov July 15, 2025 11:41
@kgogov
Copy link
Member

kgogov commented Jul 17, 2025

Hi Plamen,
Nice approach tracking the transition from hidden to visible! 👍
I tested this with a toolbar inside a dialog, but unfortunately it's not working as expected for me either. The overflow calculation still doesn't seem to trigger properly when the dialog opens.
Also we should definitely add a test case for this scenario:

it("should handle overflow correctly when toolbar becomes visible from hidden state", async () => {
    // Test toolbar inside a closed dialog that gets opened
    // Verify overflow button and item distribution work correctly
});

Image

I changed my approach for fixing the issue.

Could you try the fix?

I tried numerous ways to write a test for our case but it proves almost impossible to hit this exact case in test environment.

May you could suggest a test ?

describe("Toolbar in Dialog", () => {
	it("Should correctly process overflow layout when rendered inside a dialog", () => {
		cy.viewport(400, 600);

		cy.mount(
			<div>
				<Button id="open-dialog-button" onClick={() => {
					const dialog = document.getElementById("dialog") as Dialog;
					dialog.open = true;
				}}>Open Dialog</Button>

				<Dialog id="dialog">
					<Toolbar id="toolbar-in-dialog">
						<ToolbarButton icon={add} text="Plus" design="Default"></ToolbarButton>
						<ToolbarButton icon={employee} text="Hire"></ToolbarButton>
						<ToolbarSeparator></ToolbarSeparator>
						<ToolbarButton icon={add} text="Add"></ToolbarButton>
						<ToolbarButton icon={decline} text="Decline"></ToolbarButton>
						<ToolbarSpacer></ToolbarSpacer>
						<ToolbarButton icon={add} text="Append"></ToolbarButton>
						<ToolbarButton icon={employee} text="More"></ToolbarButton>
						<ToolbarButton icon={decline} text="Extra"></ToolbarButton>
						<ToolbarButton icon={add} text="Final"></ToolbarButton>
						<ToolbarButton icon={employee} text="Last"></ToolbarButton>
						<ToolbarButton icon={decline} text="Final"></ToolbarButton>
						<ToolbarButton icon={add} text="Plus"></ToolbarButton>
					</Toolbar>
				</Dialog>
			</div>
		);

		// Open dialog
		cy.get("#open-dialog-button").click();
		cy.get<Dialog>("#dialog").ui5DialogOpened();

		// Verify toolbar is rendered inside the dialog
		cy.get("#toolbar-in-dialog")
			.should("exist")
			.should("be.visible");

		// Check that overflow processing has occurred by verifying overflow button exists and is visible
		// Since we have many items in a constrained width, some should overflow
		cy.get("#toolbar-in-dialog")
			.shadow()
			.find(".ui5-tb-overflow-btn")
			.should("exist")
			.should("not.have.class", "ui5-tb-overflow-btn-hidden");
	});
});

Copy link
Member

@kgogov kgogov left a comment

Choose a reason for hiding this comment

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

Your code works great! Maybe you could try adding the test I suggested?

@plamenivanov91 plamenivanov91 requested a review from kgogov July 17, 2025 14:53
@plamenivanov91 plamenivanov91 merged commit e249cf6 into SAP:main Jul 17, 2025
33 of 36 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[ui5-toolbar]: overflow is not triggered when Toolbar is placed in a Dialog
2 participants