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-button): add second (end) icon #8925

Merged
merged 4 commits into from
May 17, 2024
Merged

feat(ui5-button): add second (end) icon #8925

merged 4 commits into from
May 17, 2024

Conversation

NHristov-sap
Copy link
Contributor

@NHristov-sap NHristov-sap commented May 9, 2024

Button now can have second icon at the end

BREAKING CHANGE: iconEnd property is changed from boolean to string type and now can accept name for second/end icon.

Before:

<ui5-button icon="home" icon-end>Button</ui5-button>

Now:

<ui5-button end-icon="home">Button</ui5-button>

or

<ui5-button icon="employee" end-icon="home">Button</ui5-button>

@vladitasev
Copy link
Contributor

I think the icon-end property must be deprecated and a new end-icon property added. The icon-end name has boolean semantics ("icon is at the end" - true/false) while the end-icon name would mean "icon which is at the end" or "end icon" (noun). Same for the private property: has-icon-end -> has-end-icon

Then, it would be more straightforward for users, f.e. <ui5-button icon="employee" end-icon="add>Test</ui5-button>

Avatar.ts has for example fallback-icon (the word "icon" is again at the end)

@NHristov-sap
Copy link
Contributor Author

I think the icon-end property must be deprecated and a new end-icon property added.

I hope you mean 'icon-end' to be completely removed, as if it stays (even deprecated), and we introduce new property 'end-icon', there might be situations where there are two icons at the end. Also, technically it will be very difficult to maintain both options at the same time due to the way 'icon-end' works now.

@NHristov-sap NHristov-sap marked this pull request as ready for review May 17, 2024 07:38
@NHristov-sap NHristov-sap merged commit 2e97c03 into main May 17, 2024
10 checks passed
@NHristov-sap NHristov-sap deleted the BL_button_icons branch May 17, 2024 07:40
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

3 participants