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(ui5-menu-item): add endContent slot #9077

Merged
merged 8 commits into from
Jun 3, 2024
Merged

Conversation

NHristov-sap
Copy link
Contributor

New slot endContent is introduced in ui5-menu-item component. This slot accepts components that are displayed at the end of the Menu Item if it does not have sub-menu. If there are components slotted and also additionalText is being set, the additionalText will not be shown as well.

It is strongly recommended to slot only icon-only ui5-button, ui5-icon or ui5-link components.

BGSOFUIBALKAN-8160
#6350

Copy link
Member

Choose a reason for hiding this comment

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

From the list item's side, the changes look fine to me.

@NHristov-sap NHristov-sap requested a review from unazko May 30, 2024 09:12
Copy link
Contributor

@vladitasev vladitasev left a comment

Choose a reason for hiding this comment

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

This API looks a bit strange: you have 2 slots, but they are mutually exclusive, and also an additionalText property that is exclusive with the slots.

  1. If you have passed submenu items, the "arrow" icon will be rendered
  2. if you have passed end content, it will be displayed
  3. lowest prio - the additionalText property will be displayed (this one is documented properly explaining that it's exclusive with the submenu)

There are 2 solutions:

  • document the slots as mutually exclusive
  • use the same "default" slot for both purposes: if you pass menu items, then work as before. But if you pass other content, act as the current "endContent". But maybe this is not ideal because what would happen if people pass a mix of submenus and arbitrary content? We'd still have to document how it would behave in that scenario. So maybe the current implementation is better

packages/main/src/MenuItem.hbs Show resolved Hide resolved
packages/main/src/MenuItem.ts Outdated Show resolved Hide resolved
packages/main/src/MenuItem.ts Outdated Show resolved Hide resolved
@NHristov-sap
Copy link
Contributor Author

There are 2 solutions:

  • document the slots as mutually exclusive
  • use the same "default" slot for both purposes: if you pass menu items, then work as before. But if you pass other content, act as the current "endContent". But maybe this is not ideal because what would happen if people pass a mix of submenus and arbitrary content? We'd still have to document how it would behave in that scenario. So maybe the current implementation is better

Using one slot for all three purposes is not an option here, so I have updated the documentation to explain the priority of what is displayed at the end of the menu item.

Copy link
Contributor

@vladitasev vladitasev left a comment

Choose a reason for hiding this comment

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

There is a small typo ("nether" -> "neither") but it's ok to merge this change not to block progress on other menu changes.

We should consult with UA how to best describe this items > endContent > additionalText hierarchy and we can address this typo in this change.

Copy link
Contributor

@tsanislavgatev tsanislavgatev left a comment

Choose a reason for hiding this comment

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

Looks good to me, but let's add UA to review the PR. We can merge it and address the coments later, but we can use this PR for the review

@NHristov-sap NHristov-sap merged commit dc3cfde into main Jun 3, 2024
10 checks passed
@NHristov-sap NHristov-sap deleted the BL_menuitem_endcontent branch June 3, 2024 11:51
NHristov-sap added a commit that referenced this pull request Jun 7, 2024
[feat(ui5-menu-item): add endContent slot](#9077) PR introduces a regression in Tab chaining in lists because of improper marking of `keyup` event in `ui5-button` component. It was marked always but should be marked only if event is fired by Space key. In addition, there is redundant check in ListItemBase which is removed now.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants