Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

fix(styles): Inverted styles on Button and InputSearch ignore custom themes. #931

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

masoudmanson
Copy link
Contributor

@masoudmanson masoudmanson commented Feb 14, 2025

Summary

NavigationHeader, Button, InputSearch
Github issue: Jira Ticket

The Issue

The current implementation of inverted styles in some sub-components of NavigationHeader does not respect custom theme overrides. While the default light and dark SDS themes work correctly, custom themes—such as modified color values—are ignored, causing the components to revert to the default SDS theme instead.

Affected Components

  • Button
  • ButtonIcon
  • ButtonDropdown
  • InputSearch
  • NavigationHeader

Expected Behavior

Inverted styles should adapt to custom theme overrides instead of defaulting to the SDS theme.

Next Steps

  • Investigate a fix to ensure inverted styles work correctly with custom themes.
  • If a fix isn’t feasible, consider overriding inverted style themes for affected sub-components.

Checklist

  • Default Story in Storybook
  • Test Story in Storybook
  • Tests written
  • Semantic Variables from defaultTheme.ts used wherever possible
  • If updating an existing component, depreciate flag has been used where necessary
  • ZeroHeight Documents updated
  • Chromatic build verified by @chanzuckerberg/sds-design

@masoudmanson masoudmanson self-assigned this Feb 14, 2025
@masoudmanson masoudmanson added Bug Something isn't working P0 Work in Progress This PR is a work in progress labels Feb 14, 2025
Copy link
Contributor

@tihuan tihuan left a comment

Choose a reason for hiding this comment

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

Oh awesome! The new implementation makes sense 😄 💡

Just one small question regarding ripple effect thank you, Masoud 🎉 !!

@@ -81,6 +81,9 @@ exports[`<Pagination /> Default story renders snapshot 1`] = `
width="16"
/>
</div>
<span
class="MuiTouchRipple-root css-8je8zh-MuiTouchRipple-root"
Copy link
Contributor

Choose a reason for hiding this comment

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

not sure if the Material UI ripple effect actually shows up, but wanted to flag this to make this this is expected to be here!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Wow, sharp eyes as always, @tihuan! 😯 Great catch!
I’ll make sure to disable the ripple effect. 😬

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is strange! 🥲

I’ve disabled all ripple effects in the theme file, yet the TouchRipple element is still being added to the components! It seems like a bug with MUI, and hopefully, it’ll be resolved once we upgrade to MUI v6.

https://stackoverflow.com/questions/72341110/mui-v5-8-0-disableripple-button-prop-doesnt-have-an-affect

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh awesome! Thanks for looking into it 😄 ❤️ So MUI adds the class due to the bug, but it doesn't actually have ripple effect? If so, then I think that's good enough and we can see if the upgrade will get rid of it 💪 Thanks so much!

Copy link

@clarsen-czi clarsen-czi left a comment

Choose a reason for hiding this comment

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

LGTM!! Thanks @masoudmanson!

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
Bug Something isn't working P0 Work in Progress This PR is a work in progress
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants