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): implement sap_horizon theme #4126

Merged
merged 9 commits into from
Oct 26, 2021
Merged

Conversation

Todor-ads
Copy link
Member

@Todor-ads Todor-ads commented Oct 19, 2021

feat(ui5-button): implement sap_horizon theme
feat(ui5-toggle button): implement sap_horizon theme

@@ -9,4 +9,5 @@
--_ui5_button_icon_font_size: 1rem;
--_ui5_button_emphasized_font_weight: bold;
--_ui5_button_text_shadow: none;
--sap_button_emphasized_focused_border: 0.0625rem dotted var(--sapContent_ContrastFocusColor);
Copy link
Member

Choose a reason for hiding this comment

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

all params start with --_ui5_button, not --sap_button

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.

Button

The focus state on warning/attention button seems wrong
Screenshot 2021-10-19 at 15 16 15

Down state for all types seems wrong - the respective outline is missing.

@tsanislavgatev
Copy link
Contributor

The focus on fiori_3 regular toggle button, when in active state is invisible.

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.

There is a misalignment between two focuses of the emphasized button

@tsanislavgatev tsanislavgatev self-requested a review October 25, 2021 12:26
tsanislavgatev
tsanislavgatev previously approved these changes Oct 25, 2021
@ilhan007
Copy link
Member

Can you check if the focus color when button is pressed down is correct in Fiori_3 (and FIori_3 Dark as well), because it changed with the current PR, but I guess it should not.
https://user-images.githubusercontent.com/15702139/138696873-7b310b2e-d644-4f36-94c7-fdc426ccc1fb.mov

Which of the two hover states over a default type button is correct

openui5 nightly
Screenshot 2021-10-25 at 15 39 03

in this PR
Screenshot 2021-10-25 at 15 39 17

@ilhan007 ilhan007 requested a review from terezamch October 25, 2021 12:52
@tsanislavgatev tsanislavgatev dismissed their stale review October 25, 2021 13:31

Changes needed

@tsanislavgatev
Copy link
Contributor

@ilhan007 The focus is indeed broken in the PR, the colors on press should be inverted (black to white).
For the question on the button. Hover should be in black color, the down(active) state should be in blue.

@terezamch
Copy link
Contributor

The hover should only apply shadow. the blue text color is applied when the button becomes active.

@tsanislavgatev
Copy link
Contributor

After the last changes, it looks good. Checked the visual and with ui5. I think the button is in good state at the moment, and the comments made, were fixed. @ilhan007

@ilhan007 ilhan007 dismissed their stale review October 26, 2021 08:58

outdated

@ilhan007
Copy link
Member

After the last changes, it looks good. Checked the visual and with ui5. I think the button is in good state at the moment, and the comments made, were fixed. @ilhan007

All right, I won't test further, you can merge the PR whenever you decide

@tsanislavgatev tsanislavgatev merged commit 90483e2 into master Oct 26, 2021
@tsanislavgatev tsanislavgatev deleted the button_next branch October 26, 2021 10:49
Todor-ads added a commit that referenced this pull request Oct 28, 2021
feat(ui5-button): implement sap_horizon theme
feat(ui5-toggle-button): implement sap_horizon theme
Todor-ads added a commit that referenced this pull request Oct 28, 2021
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.

4 participants