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

Add MUI theme config to fix Sidebar and Tabs color #960

Merged
merged 1 commit into from
Feb 19, 2024

Conversation

debsmita1
Copy link
Member

Which issue(s) does this PR fix

Selected Tab (dark theme)
Screenshot 2024-02-09 at 11 24 37 PM

Tab when hovered (dark theme)
Screenshot 2024-02-09 at 11 24 24 PM

Selected Tab (light theme)
Screenshot 2024-02-09 at 11 25 31 PM

Tab when hovered (light theme)
Screenshot 2024-02-09 at 11 25 38 PM

Nav when hovered (light theme)
Screenshot 2024-02-09 at 11 27 41 PM

Nav when hovered (dark theme)
Screenshot 2024-02-09 at 11 29 17 PM

@debsmita1 debsmita1 requested a review from a team as a code owner February 9, 2024 18:00
Copy link

changeset-bot bot commented Feb 9, 2024

🦋 Changeset detected

Latest commit: 12f07a1

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 9, 2024

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

Copy link
Contributor

janus-idp bot commented Feb 9, 2024

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

@invincibleJai
Copy link
Member

/cc @ShiranHi
/cc @jerolimov

@ShiranHi
Copy link

Thank you @debsmita1!, I have few comments

  1. Tabs (I compared the screenshots with the design here)
    I think we don't display the gray background color when hovering over the tab or when it's selected

  2. Navigation (I compared the screenshots with the design here)
    We shouldn't display the underline below the text when hovering

  3. In dark mode, the blue color appears brighter. We use the same main color in both scenarios, both for the tab line color and the blue line on the navigation

@debsmita1
Copy link
Member Author

Thank you @debsmita1!, I have few comments

  1. Tabs (I compared the screenshots with the design here)
    I think we don't display the gray background color when hovering over the tab or when it's selected

The tabs component that you see being used in plugins is a Backstage’s HeaderTab component
 and the hover color is the same as the background color of the app.

    tabRoot: {
      '&:hover': {
        backgroundColor: theme.palette.background.default,
        color: theme.palette.text.primary,
      },
    },
  1. Navigation (I compared the screenshots with the design here)
    We shouldn't display the underline below the text when hovering

Similarly, the nav component is Backstage's SidebarItem component and there's no prop available that allows removing the underline

  1. In dark mode, the blue color appears brighter. We use the same main color in both scenarios, both for the tab line color and the blue line on the navigation

Fixed it, using '#0066CC' as mentioned in the slide

Screenshot 2024-02-13 at 12 46 08 AM

Screenshot 2024-02-13 at 12 04 02 PM

Copy link
Contributor

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

Copy link
Contributor

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

@ShiranHi
Copy link

The tabs component that you see being used in plugins is a Backstage’s HeaderTab component
 and the hover color is the same as the background color of the app.

Can we do the same? so the hover color will be the same to the background?

Similarly, the nav component is Backstage's SidebarItem component and there's no prop available that allows removing the underline

I am not sure why we have that underline on the navigation when hovering, on the Backstage demo env they don't show any underline when hovering

@debsmita1 debsmita1 force-pushed the nav-tab branch 2 times, most recently from d369688 to 96898d0 Compare February 14, 2024 06:34
@debsmita1
Copy link
Member Author

The tabs component that you see being used in plugins is a Backstage’s HeaderTab component
> and the hover color is the same as the background color of the app.

Can we do the same? so the hover color will be the same to the background?

So an example to show you what happens if I change the background default color (theme.palette.background.default)
Screenshot 2024-02-14 at 11 25 16 AM

In the light theme, if I have to match the hover color to the selected tab color it will look like this

Screenshot 2024-02-14 at 11 31 58 AM

dark theme would look like

Screenshot 2024-02-14 at 11 33 49 AM

Similarly, the nav component is Backstage's SidebarItem component and there's no prop available that allows removing the underline

I am not sure why we have that underline on the navigation when hovering, on the Backstage demo env they don't show any underline when hovering

Fixed it, the underline was added by us. Thank you sharing the backstage demo

Screen.Recording.2024-02-14.at.12.03.20.PM.mov

Copy link
Contributor

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

@gashcrumb
Copy link
Member

gashcrumb commented Feb 14, 2024

This looks really good! The only thing that's bugging me is the border line under the tabs, for both light and dark it there's a margin on the left-hand side leaving a space but on the right-hand side of the page there's no space. I tried capturing a more narrow window to help highlight this:

image

Shows up better in dark mode and I think maybe the border color could be too bright in this variant also:

image

@debsmita1
Copy link
Member Author

debsmita1 commented Feb 14, 2024

This looks really good! The only thing that's bugging me is the border line under the tabs, for both light and dark it there's a margin on the left-hand side leaving a space but on the right-hand side of the page there's no space. I tried capturing a more narrow window to help highlight this:

image

Shows up better in dark mode and I think maybe the border color could be too bright in this variant also:

image

@gashcrumb The HeaderTab component adds this padding on the left

const useStyles = makeStyles(
  theme => ({
    tabsWrapper: {
      gridArea: 'pageSubheader',
      backgroundColor: theme.palette.background.paper,
      paddingLeft: theme.spacing(3), <--------
      minWidth: 0,
    },
    ...

and the createUnifiedTheme util doesn't expose any option to update the spacing

@gashcrumb
Copy link
Member

@gashcrumb The HeaderTab component adds this padding on the left
and the createUnifiedTheme util doesn't expose any option to update the spacing

I don't know if maybe trying to override it using something like this could work, but if you like this could be investigated later as a separate thing.

@debsmita1 debsmita1 force-pushed the nav-tab branch 2 times, most recently from 5ca8ded to e349cb0 Compare February 15, 2024 06:32
@debsmita1
Copy link
Member Author

@gashcrumb The HeaderTab component adds this padding on the left
and the createUnifiedTheme util doesn't expose any option to update the spacing

I don't know if maybe trying to override it using something like this could work, but if you like this could be investigated later as a separate thing.

This worked! Thank you @gashcrumb !!
Screenshot 2024-02-15 at 11 59 57 AM

Copy link
Contributor

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

@ShiranHi
Copy link

looks good @debsmita1 ! 🙂

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.

I have two small improvement ideas.

@invincibleJai I'm also fine to get this in now and improve this later. Feel free to add approval, it's def. the right direction and btw it will create a conflict with #957

/lgtm

@invincibleJai
Copy link
Member

Thanks @jerolimov

@debsmita1 PTAL at open comments and would be nice if we could handle those in the same PR. LMK

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.

Looks good 👍

/lgtm
/approve

@openshift-ci openshift-ci bot added the lgtm label Feb 19, 2024
@invincibleJai
Copy link
Member

/approve

@christoph-jerolimov
Copy link
Member

/retest

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.

/lgtm

@openshift-ci openshift-ci bot added lgtm and removed lgtm labels Feb 19, 2024
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

@kadel
Copy link
Member

kadel commented Feb 19, 2024

/lgtm
it was just update with main branch

Copy link

openshift-ci bot commented Feb 19, 2024

[APPROVALNOTIFIER] This PR is APPROVED

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

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:
  • OWNERS [invincibleJai,kadel]

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

Copy link
Contributor

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

@openshift-merge-bot openshift-merge-bot bot merged commit da6b742 into redhat-developer:main Feb 19, 2024
6 checks passed
kadel pushed a commit to kadel/backstage-showcase that referenced this pull request Feb 20, 2024
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