Skip to content

chore(ui5-toolbar-select): value and label added #11972

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 14 commits into
base: main
Choose a base branch
from
Open
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
34 changes: 34 additions & 0 deletions packages/main/cypress/specs/Toolbar.cy.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -492,6 +492,40 @@ describe("Toolbar Select", () => {
});
});
});

it("Should correctly handle the 'value' property and 'label' slot of ToolbarSelect", () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's move this test to a ToolbarSelect.cy.tsx file and test the value and slot members separately.

Also add few more tests for the value property, for example changing the value should change the selection or passing a value that has no corresponding option, etc.

Copy link
Contributor

Choose a reason for hiding this comment

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

Lets also test with the ui5-toolbar-select-option component

// Mount the Toolbar with a ToolbarSelect component
cy.mount(
<Toolbar>
<ToolbarSelect value="Option 2">
<span slot="label">Select an Option:</span>
<ToolbarSelectOption>Option 1</ToolbarSelectOption>
<ToolbarSelectOption>Option 2</ToolbarSelectOption>
<ToolbarSelectOption>Option 3</ToolbarSelectOption>
</ToolbarSelect>
</Toolbar>
);

// Verify the initial value of the ToolbarSelect
cy.get("ui5-select", { includeShadowDom: true })
.should("have.attr", "value", "Option 2");

// Verify the label slot content
cy.get("ui5-toolbar-select")
.find("span[slot='label']")
.should("contain.text", "Select an Option:");

// Change the value of the ToolbarSelect
cy.get("ui5-select", { includeShadowDom: true })
.realClick()
.find("ui5-option")
.contains("Option 3")
.realClick();

// Verify the updated value of the ToolbarSelect
cy.get("ui5-select", { includeShadowDom: true })
.should("have.attr", "value", "Option 3");
});
});

describe("Toolbar Button", () => {
Expand Down
48 changes: 47 additions & 1 deletion packages/main/src/ToolbarSelect.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import slot from "@ui5/webcomponents-base/dist/decorators/slot.js";
import event from "@ui5/webcomponents-base/dist/decorators/event-strict.js";
import type ValueState from "@ui5/webcomponents-base/dist/types/ValueState.js";
import ToolbarSelectCss from "./generated/themes/ToolbarSelect.css.js";
import type Select from "./Select.js";

// Templates
import ToolbarSelectTemplate from "./ToolbarSelectTemplate.js";
Expand Down Expand Up @@ -87,9 +88,22 @@ class ToolbarSelect extends ToolbarItem {
* **Note:** Use the `ui5-toolbar-select-option` component to define the desired options.
* @public
*/
@slot({ "default": true, type: HTMLElement, invalidateOnChildChange: true })
@slot({
"default": true,
type: HTMLElement,
invalidateOnChildChange: true,
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need invalidateOnChildChange here?

})
options!: Array<ToolbarSelectOption>;

/**
* Defines the HTML element that will be displayed in the component input part,
* representing the selected option.
* @public
* @since 2.13.0
*/
@slot()
label!: Array<HTMLElement>;

/**
* Defines the value state of the component.
* @default "None"
Expand Down Expand Up @@ -124,6 +138,34 @@ class ToolbarSelect extends ToolbarItem {
@property()
accessibleNameRef?: string;

/**
* Defines the value of the component:
*
* @public
* @default ""
* @since 2.13.0
*/
@property()
set value(newValue: string) {
if (this.select && this.select.value !== newValue) {
const selectedOption = this.select.options.find(option => option.textContent === newValue);
this.select.value = newValue;
selectedOption && this.fireDecoratorEvent("change", { targetRef: this.select, selectedOption });
Copy link
Contributor

Choose a reason for hiding this comment

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

As of now the select isn't firing the change event if the value changes. In future, if the selects starts firing it we have a handler below that will catch and re-fire it from the toolbar select. If this is a requirement now, we can add it to the select itself.

}
this._value = newValue;
}

get value(): string | undefined {
return this.select ? this.select.value : this._value;
}

get select(): Select | null {
return this.shadowRoot!.querySelector<Select>("[ui5-select]");
}

// Internal value storage, in case the composite select is not rendered on the the assignment happens
_value: string = "";

onClick(e: Event): void {
e.stopImmediatePropagation();
const prevented = !this.fireDecoratorEvent("click", { targetRef: e.target as HTMLElement });
Expand Down Expand Up @@ -174,6 +216,10 @@ class ToolbarSelect extends ToolbarItem {
width: this.width,
};
}

get hasCustomLabel() {
return !!this.label.length;
}
}

ToolbarSelect.define();
Expand Down
8 changes: 6 additions & 2 deletions packages/main/src/ToolbarSelectTemplate.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ export default function ToolbarSelectTemplate(this: ToolbarSelect) {
<Select
class={this.classes.root}
style={this.styles}
value={this.value}
data-ui5-external-action-item-id={this._id}
valueState={this.valueState}
disabled={this.disabled}
Expand All @@ -17,6 +18,10 @@ export default function ToolbarSelectTemplate(this: ToolbarSelect) {
onOpen={(...args) => this.onOpen(...args)}
onChange={(...args) => this.onChange(...args)}
>
{this.hasCustomLabel &&
<slot name="label" slot="label">
</slot>
}
{this.options.map((option, index) => (
<Option
selected={option.selected}
Expand All @@ -25,6 +30,5 @@ export default function ToolbarSelectTemplate(this: ToolbarSelect) {
{option.textContent}
</Option>
))}
</Select>
);
</Select>);
}
4 changes: 4 additions & 0 deletions packages/main/test/pages/Toolbar.html
Original file line number Diff line number Diff line change
Expand Up @@ -71,6 +71,10 @@
<ui5-toolbar-button icon="add" text="Right 1"></ui5-toolbar-button>
<ui5-toolbar-button icon="employee" text="Right 4"></ui5-toolbar-button>
<ui5-toolbar-select id="toolbar-select">
<div class="selectLabel" slot="label">
<span id="mainText">Skirt [3]</span>
<ui5-avatar id="avatar" size="XS" initials="S"></ui5-avatar>
</div>
<ui5-toolbar-select-option>1</ui5-toolbar-select-option>
<ui5-toolbar-select-option selected>2</ui5-toolbar-select-option>
<ui5-toolbar-select-option>3</ui5-toolbar-select-option>
Expand Down
14 changes: 13 additions & 1 deletion packages/main/test/pages/ToolbarSelect.html
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
<head>
<meta charset="utf-8" />

<title>ToolbarButton</title>
<title>ToolbarSelect</title>
<meta
name="viewport"
content="width=device-width, initial-scale=1.0, maximum-scale=1.0, user-scalable=no"
Expand Down Expand Up @@ -34,6 +34,15 @@
<ui5-option selected>2</ui5-option>
<ui5-option>3</ui5-option>
</ui5-toolbar-select>

<!-- with label -->

<ui5-toolbar-select id="labeledSelect" value="15pt">
<span slot="label" id="labeledSelectLabel">18pt</span>
<ui5-option>12pt</ui5-option>
<ui5-option>15pt</ui5-option>
<ui5-option>21pt</ui5-option>
</ui5-toolbar-select>
</ui5-toolbar>

<script>
Expand All @@ -59,6 +68,9 @@
select.addEventListener("ui5-click", (event) => {
clicked.value++;
});
labeledSelect.addEventListener("ui5-change", (event) => {
labeledSelectLabel.textContent = event.detail.selectedOption.textContent
})
</script>
</body>
</html>
Loading