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

refactor(ui5-option): make options physical elements #8903

Merged
merged 32 commits into from
May 21, 2024

Conversation

dobrinyonkov
Copy link
Contributor

@dobrinyonkov dobrinyonkov commented May 7, 2024

Removes the ui5-select-menu and ui5-select-menu-option components.

BREAKING CHANGE: The ui5-select-menu and ui5-select-menu-option components are removed. Custom options can now be created using the ui5-option-custom, directly placed inside the default slot of the ui5-select

SelectMenu & SelectMenuOption

Changed item Old New
SelectMenu ui5-select-menu removed
SelectMenuOption ui5-select-menu-option ui5-option-custom

If you have previously used the ui5-select-menu and ui5-select-menu-option:

<ui5-select menu="selectMenu"></ui5-select>

<ui5-select-menu id="selectMenu">
    <ui5-select-menu-option>
        <div class="optionContent">custom</div>
    </ui5-select-menu-option>
</ui5-select-menu>

Now use just ui5-select and ui5-option-custom instead:

<ui5-select>
    <ui5-option-custom>
        <div class="optionContent">custom</div>
    </ui5-option-custom>
</ui5-select>

Select

Changed item Old New
property menu removed

The menu property of the ui5-select is removed.

Related to #8461, #7887

@dobrinyonkov dobrinyonkov changed the title POC refactor(ui5-option): make options physical elements refactor(ui5-option): make options physical elements May 8, 2024
packages/main/src/Option.ts Outdated Show resolved Hide resolved
packages/main/src/OptionCustom.ts Show resolved Hide resolved
template: OptionBaseTemplate,
renderer: litRender,
})
class OptionBase extends UI5Element implements IOption {
Copy link
Contributor

Choose a reason for hiding this comment

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

IOption seems to be redundant since we have OptionBase class

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I removed the IOption interface here, but we might still need something for the Select.ts file. There we are using the icon property of the Option which is missing in the OptionBase and as well as some other fields in the SelectPopover.hbs as additionalText, isCustom getter. IOption interface there combines all the stuff the select needs from both the custom option and the option.

packages/main/src/themes/OptionCustom.css Outdated Show resolved Hide resolved
packages/main/src/SelectPopover.hbs Outdated Show resolved Hide resolved
packages/main/src/SelectPopover.hbs Outdated Show resolved Hide resolved
packages/main/src/SelectPopover.hbs Outdated Show resolved Hide resolved
packages/main/src/SelectPopover.hbs Outdated Show resolved Hide resolved
packages/main/src/Select.ts Outdated Show resolved Hide resolved
packages/main/src/Select.ts Outdated Show resolved Hide resolved
packages/main/src/Select.ts Outdated Show resolved Hide resolved
@nnaydenow nnaydenow requested a review from vladitasev May 8, 2024 09:42
packages/main/src/Option.ts Outdated Show resolved Hide resolved
packages/main/src/Select.ts Show resolved Hide resolved
packages/main/src/Select.ts Outdated Show resolved Hide resolved
Copy link
Contributor

@nnaydenow nnaydenow left a comment

Choose a reason for hiding this comment

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

Minor changes, otherwise it looks and works good to me.

packages/main/src/Select.ts Outdated Show resolved Hide resolved
packages/main/src/Select.ts Outdated Show resolved Hide resolved
@dobrinyonkov dobrinyonkov force-pushed the ui5-option-physical branch 2 times, most recently from 3c3beb0 to 53c35d3 Compare May 16, 2024 07:40
Copy link
Contributor

@nnaydenow nnaydenow left a comment

Choose a reason for hiding this comment

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

  • Add notes for additionalTextState, description, iconEnd, image, movable, accessibleName, deleteButton, imageContent. Also documentaiton of selected should be adjusted. Same apply for the OptionCustom class and for some of the parts.

packages/main/src/Option.ts Outdated Show resolved Hide resolved
vladitasev
vladitasev previously approved these changes May 21, 2024
@dobrinyonkov dobrinyonkov merged commit 8d6fac7 into main May 21, 2024
9 checks passed
@dobrinyonkov dobrinyonkov deleted the ui5-option-physical branch May 21, 2024 08:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants