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(ui): add pf5 button style config #957

Conversation

ciiay
Copy link
Member

@ciiay ciiay commented Feb 8, 2024

Description

A theme config for a Material-UI Button (v4) to look as similar as possible to a PF5 Button.

Which issue(s) does this PR fix

  • Fixes RHIDP-1163: Add MUI theme config so that Buttons looks similar to PF5

PR acceptance criteria

Please make sure that the following steps are complete:

  • GitHub Actions are completed and successful
  • Unit Tests are updated and passing
  • E2E Tests are updated and passing
  • Documentation is updated if necessary (requirement for new features)
  • Add a screenshot if the change is UX/UI related

How to test changes / Special notes to the reviewer

Light theme buttons:
light_buttons

Contained_button
light_contained_button_text_button
Contained_button_hover
light_contained_button_hover
Contained_button_disabled
light_contained_button_disabled
Outlined_button
light_outlined_button
Outlined_button_hover
light_outlined_button_hover
Text_button
light_text_button
Text_button_hover
light_text_button_hover

Dark theme buttons:
Contained button and text button:

updated_contained_button_in_dark_theme.mov

Contained_button_disabled
dark_contained_button_disabled
Outline_button
dark_outline_button
Outline_button_hover
dark_outline_button_hover

Dark theme palette reference:
https://www.patternfly.org/developer-resources/dark-theme-handbook/
https://github.com/patternfly/patternfly/blob/main/src/patternfly/sass-utilities/themes/dark/scss-variables.scss
https://github.com/patternfly/patternfly/blob/main/src/patternfly/sass-utilities/themes/dark/colors.scss

@ciiay ciiay requested a review from a team as a code owner February 8, 2024 22:09
Copy link

changeset-bot bot commented Feb 8, 2024

🦋 Changeset detected

Latest commit: 7d657d2

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
app Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

Copy link
Contributor

github-actions bot commented Feb 8, 2024

The image is available at: quay.io/janus-idp/backstage-showcase:pr-957!

Copy link
Contributor

janus-idp bot commented Feb 8, 2024

backstage-showcase Tests on commit db274a0 finished successfully.
View test log

Copy link
Contributor

github-actions bot commented Feb 9, 2024

The image is available at: quay.io/janus-idp/backstage-showcase:pr-957!

@ciiay ciiay requested a review from gashcrumb February 9, 2024 22:26
@ShiranHi
Copy link

@ciiay that looks great, thank you!
I have 2 questions:

  1. It appear that the button's dark theme color differs between light and dark modes? From what I saw, it should remain the same shade of blue, shouldn't it?
image
  1. Is there a way to improve the visibility of the disabled "Contained_button_disabled" button in dark mode? It seems to blend too much with the background color. Is there something we can do? could we adjust the background color to make the button stand out more?
image2

@ciiay
Copy link
Member Author

ciiay commented Feb 12, 2024

Hi @ShiranHi , thanks for the review.

  1. According to the Blue color palette on PF5 dark theme handbook documentation, the primary blue (blue-300) gets updated for the dark theme. You can also check the updated color code in here and here. Let me know if we want to keep the original primary blue in dark theme. Here's a preview of it.
regular_primary_blue_on_dark_theme

Comparing with the updated primary blue:
with_updated_primary_blue

  1. Our current card background color doesn't match the PF5 value. @jerolimov and I discussed about this issue and we're going to create a ticket to update the card style to match PF5. Then the disabled contained button would look good. Here's a preview.
    PF5 style in dark theme:
updated_card_color Current style in dark theme: current_card_color

Also there's a demo for PF5 style we can refer, and the sidebar menu color in dark theme looks pretty different from our current one. You can toggle theme using the bottom right button. Let us know if we want to match it or keep our current style. Thanks.

Copy link
Member

@gashcrumb gashcrumb left a comment

Choose a reason for hiding this comment

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

This looks great! If the dark theme card background is a separate issue I think this is good to go. 👍

Well, and probably this will need a changeset added too @ciiay

Signed-off-by: Yi Cai <[email protected]>
@openshift-ci openshift-ci bot removed the lgtm label Feb 12, 2024
@ciiay ciiay requested a review from gashcrumb February 12, 2024 18:43
@openshift-ci openshift-ci bot added the lgtm label Feb 12, 2024
Copy link
Contributor

The image is available at: quay.io/janus-idp/backstage-showcase:pr-957!

@ShiranHi
Copy link

  1. According to the Blue color palette on PF5 dark theme handbook documentation, the primary blue (blue-300) gets updated for the dark theme. You can also check the updated color code in here and here. Let me know if we want to keep the original primary blue in dark theme. Here's a preview of it.

I see, then let's follow the PF guidelines. I am a bit confused because when I switch between the bright and dark mode on the PF page (top right corner), the blue color stays the same.

Our current card background color doesn't match the PF5 value. @jerolimov and I discussed about this issue and we're going to create a ticket to update the card style to match PF5. Then the disabled contained button would look good. Here's a preview.

Thanks for the update, the card with the PF5 style in dark theme looks great!

Also there's a demo for PF5 style we can refer, and the sidebar menu color in dark theme looks pretty different from our current one. You can toggle theme using the bottom right button. Let us know if we want to match it or keep our current style. Thanks.

Yes, let's follow this demo. I think this is what @debsmita1 is working on in this PR

@openshift-ci openshift-ci bot removed the lgtm label Feb 13, 2024
Copy link
Contributor

The image is available at: quay.io/janus-idp/backstage-showcase:pr-957!

@ciiay
Copy link
Member Author

ciiay commented Feb 13, 2024

Hi @ShiranHi , as confirmed with the PF team, the contained button background stays the same color in dark theme as the light theme. I have updated a screen video to show the updated style. Here's how it looks now.
updated_contained_button_bg_color
Hover:
updated_contained_button_bg_color_hover

@openshift-ci openshift-ci bot added the lgtm label Feb 14, 2024
@openshift-ci openshift-ci bot removed the lgtm label Feb 14, 2024
Signed-off-by: Yi Cai <[email protected]>
@ciiay ciiay requested a review from gashcrumb February 14, 2024 17:46
@openshift-ci openshift-ci bot added the lgtm label Feb 14, 2024
Copy link
Contributor

The image is available at: quay.io/janus-idp/backstage-showcase:pr-957!

@ShiranHi
Copy link

@ciiay thanks for making those adjustments! lgtm

@christoph-jerolimov
Copy link
Member

christoph-jerolimov commented Feb 16, 2024

Hi, this looks like a step into the right direction. Here are two recordings where I've tested MUI v4 and v5 buttons with the default Janus and the RHDH theme.

Main branch:

before.mp4

With this PR:

after.mp4

I saw some minor issues:

  1. The buttons still have the ripple effect; there is nothing wrong with keeping them for now. If we/you want to disable it, please check that there is a focus indicator.
  2. The shadow is only removed from primary buttons, not secondary ones.
  3. Is the sensory button color not changed in dark mode?

In the long term, we should use the palette colors from the team instead of creating a new palette of colors!

For me, it's fine to merge this because we also didn't use the primary color from app-config.yaml for buttons before.

@christoph-jerolimov
Copy link
Member

To test if I can add approval ...

/approve
/lgtm cancel

@invincibleJai
Copy link
Member

/approve

Thanks @jerolimov, added approval and feel free to add lgtm when ready

Copy link

Quality Gate Passed Quality Gate passed

Issues
0 New issues

Measures
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarCloud

@ciiay
Copy link
Member Author

ciiay commented Feb 19, 2024

@jerolimov Thanks for the review. I have updated this PR to address your feedback. Please take a look.

Copy link
Contributor

The image is available at: quay.io/janus-idp/backstage-showcase:pr-957!

Copy link
Member

@christoph-jerolimov christoph-jerolimov left a comment

Choose a reason for hiding this comment

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

Thanks for the update! Lgtm! 👍

/lgtm

@openshift-ci openshift-ci bot added the lgtm label Feb 21, 2024
Copy link

openshift-ci bot commented Feb 21, 2024

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: gashcrumb, invincibleJai, jerolimov

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-merge-bot openshift-merge-bot bot merged commit 37fc476 into redhat-developer:main Feb 21, 2024
7 checks passed
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.

6 participants