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

fix(ui5-split-button): fix split button when in wrapper #8037

Merged
merged 8 commits into from
Jan 17, 2024

Conversation

hinzzx
Copy link
Contributor

@hinzzx hinzzx commented Dec 21, 2023

When our <ui5-split-button> is wrapped and a width is set to the wrapper, our <ui5-split-button> is not correctly displayed.

With this change, we fix that issue and we also adopt the usage of CSS Logical Props, removing redunancies and reducing our CSS code significantly.

Before:

image

After:

image

Fixes: #7659

@unazko unazko self-requested a review December 22, 2023 08:38
unazko
unazko previously approved these changes Dec 22, 2023
Copy link
Contributor

@unazko unazko 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.

@hinzzx hinzzx marked this pull request as ready for review January 3, 2024 07:00
Copy link
Member

@ilhan007 ilhan007 left a comment

Choose a reason for hiding this comment

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

Did not test it, in respect to code - looks good to me.

Something that I noticed while reviewing (not directly related to the change, but still related to the Split Button styles),
that you can remove a lot of RTL styles by replacing border-left, border-right, border-* with their CSS logical props equivalents as shown in the table:

https://developer.mozilla.org/en-US/docs/Web/CSS/CSS_logical_properties_and_values/Margins_borders_padding
Screenshot 2024-01-04 at 10 57 21

@hinzzx
Copy link
Contributor Author

hinzzx commented Jan 4, 2024

Hi @ilhan007 ,

It is currently WIP regarding to the redunant RTL params. Thanks for pointing it out though. There are also few more, styling issues I would have to address as well.

@nnaydenow
Copy link
Contributor

Works okey. Found only 1 minor misalignment with border in all themes (except sap_horizon and sap_horizon_dark)
image

@unazko
Copy link
Contributor

unazko commented Jan 12, 2024

Hello @hinzzx, @ilhan007, @nnaydenow,

I've identified the following two issues, which aren't related to the reported incident at all as I do not know what is the scope of the PR any longer:

  • Issue rtl and horizon on emphasized button with active state
    SplitButton_horizon

  • Issue with fiori 3 and attention button. The text color black and instead it should be semantic color
    SplitButton_fiori_3

The initial issue is found internally and looks like an edge case.
For similar cases with unrelated issues to the initial one I think it would be more practical to log separate incidents.

Best regards,
Boyan

@hinzzx
Copy link
Contributor Author

hinzzx commented Jan 15, 2024

Hello @hinzzx, @ilhan007, @nnaydenow,

  • Issue with fiori 3 and attention button. The text color black and instead it should be semantic color
    SplitButton_fiori_3

Best regards, Boyan

Hi there, as discussed offline, I just wanted to clarify that the text on the attention button in Fiori 3 is to remain black.

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.

LGTM. Tested all themes in following scenarios:

  • compact size
  • rtl
  • compact size + rtl

@hinzzx hinzzx merged commit 38861c8 into main Jan 17, 2024
8 checks passed
@hinzzx hinzzx deleted the wrapped-split-btn-fix branch January 17, 2024 09:32
ilhan007 pushed a commit that referenced this pull request Jan 19, 2024
When our <ui5-split-button> is wrapped and a width is set to the wrapper, our <ui5-split-button> is not correctly displayed.
With this change, we fix that issue and we also adopt the usage of CSS Logical Props, removing redunancies and reducing our CSS codе significantly.
tsanislavgatev pushed a commit that referenced this pull request Feb 20, 2024
When our <ui5-split-button> is wrapped and a width is set to the wrapper, our <ui5-split-button> is not correctly displayed.
With this change, we fix that issue and we also adopt the usage of CSS Logical Props, removing redunancies and reducing our CSS codе significantly.
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-split-button]: Split button is not displayed correctly when having a wrapper
4 participants