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

[Feature]: Change v9 FluentProvider's default styles #23821

Closed
1 task done
kelseyyoung opened this issue Jul 6, 2022 · 6 comments
Closed
1 task done

[Feature]: Change v9 FluentProvider's default styles #23821

kelseyyoung opened this issue Jul 6, 2022 · 6 comments

Comments

@kelseyyoung
Copy link
Contributor

Library

React Components / v9 (@fluentui/react-components)

Describe the feature that you would like added

Today the v9 FluentProvider will add some styles to its <div> that it creates. While using it we have found the need to override some of these default styles. Our team is embedding v9 components within an already existing application, not starting a new application from scratch, and so we have multiple FluentProviders in our application wrapping the relevant areas where v9 components are present. The two styles in question for this issue are lineHeight and backgroundColor

lineHeight

FluentProvider today gets a style of "lineHeight: 20px" from ...typographyStyles.body1 in useFluentProviderStyles. This style has a broad visual effect on every element within FluentProvider, including non-v9 components. In my opinion it's fairly common to style things like labels/text without a line-height property, and adding a cascading style of 20px is "too opinionated" for this component

Suggested change: remove the lineHeight style from FluentProvider

backgroundColor

FluentProvider today gets a style of backgroundColor: tokens.colorNeutralBackground1, or white in light mode, in the same styles file mentioned above. This was breaking for our application where we had a MessageBar-like component with rounded corners showing visually on top of a canvas element. The FluentProvider wrapped this MessageBar, and so we had a poor visual experience where the white background of the FluentProvider was showing when we didn't want it to

In the below image, the background of the "canvas element" was made blue to show better contrast. You can see that the first MessageBar in this picture has the white background of the FluentProvider "blocking" the blue background of the canvas element

image

Even if we tinkered with the dimensions of the FluentProvider, we may also end up with a situation like below, where the square white edges of the FluentProvider background are still visible

image

Suggested change: make the background of FluentProvider transparent

After discussion with some Fluent team members, it seems there is certainly a use case for a FluentProvider that does set default styles, including but not limited to #23626. The above suggestions are based off of our use cases and are without context into why these styles may be needed

It was also suggested that another approach might be to export two providers, one that would be used as a "root" element that dictates the look and feel of an entire app, and one that would be more transparent, an "embedded" provider perhaps. Perhaps there are even more possible solutions to satisfy multiple use cases

Have you discussed this feature with our team

Mak, Paul, Levi

Additional context

No response

Validations

  • Check that there isn't already an issue that request the same feature to avoid creating a duplicate.
@layershifter
Copy link
Member

Hey @kelseyyoung, thanks for the ideas 👍


To clear defaults that we have there are agreed with the design team 🐱 However it's kinda expected that these styles cannot fit all needs and they could be overridden. It's a bit awkward because of chicken-egg problem with React contexts (see #23853 for more context) and currently not documented, but you can override styles like with any other component:

import {
  Button,
  FluentProvider,
  teamsDarkTheme,
  makeStyles,
  FluentProviderProps,
  tokens
} from "@fluentui/react-components";
import { TextDirectionProvider } from "@griffel/react";

const useClasses = makeStyles({
  root: { backgroundColor: "transparent" }
});

const FluentProviderWithOverrides: React.FC<FluentProviderProps> = props => {
  const classes = useClasses();

  return <FluentProvider {...props} className={classes.root} />;
};

export default function App() {
  return (
    <TextDirectionProvider dir="ltr">
      {/** ⚠️ Wrapper is required to call "useClasses" hook with proper context values **/}
      <FluentProviderWithOverrides dir="ltr" theme={teamsDarkTheme}>
        {/** children... **/}
      </FluentProviderWithOverrides>
    </TextDirectionProvider>
  );
}

https://codesandbox.io/s/quirky-wozniak-l21hqn?file=/src/App.tsx


Did you have difficulties with that? Or is there another reason why style overrides cannot solve your problem and changes in Fluent UI are better from your POV?

@khmakoto
Copy link
Member

Here's another codesandbox that uses what @layershifter suggested while showing other very similar approaches that don't work (in case anyone was following the recommendation but it didn't work still).

https://codesandbox.io/s/fluentprovider-with-custom-styles-and-dir-rtl-workaround-4fq7zv?file=/src/App.tsx

@SahilTara
Copy link

SahilTara commented Dec 14, 2022

Just wondering is there any update on this work? We basically just need it to adjust z-index for all fluent providers 😁. Mostly for nested submenus since we can't adjust z-index or mount node.

@layershifter
Copy link
Member

Here's another codesandbox that uses what @layershifter suggested while showing other very similar approaches that don't work (in case anyone was following the recommendation but it didn't work still).

https://codesandbox.io/s/fluentprovider-with-custom-styles-and-dir-rtl-workaround-4fq7zv?file=/src/App.tsx

@SahilTara did you try this? ⬆️

@msft-fluent-ui-bot
Copy link
Collaborator

Because this issue has not had activity for over 150 days, we're automatically closing it for house-keeping purposes.

Still require assistance? Please, create a new issue with up-to date details.

@microsoft-github-policy-service

Because this issue has not had activity for over 150 days, we're automatically closing it for house-keeping purposes.

Still require assistance? Please, create a new issue with up-to date details.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

9 participants