Flag on theme for no global class names #4635

Markionium · 2018-04-20T18:21:16Z

Pull request checklist

Addresses an existing issue: Fixes #0000
Include a change request file using $ npm run change

Description of changes

Adds the option to disable global classNames through the theme by setting the disableGlobalClassNames flag on the theme.

The issue that this PR solves is that global css overrides are a pain and being able to exclude them from the rendered components makes the render output saver to unexpected css overrides done by other fabric users on the same webpage.

Focus areas to test

(optional)

* master: DetailsRow: Flexshrink fix (microsoft#4622) [TextField] Implemented masking (microsoft#3783) Applying package updates. ComboBox: Add any event as additional parameter to onChanged callback for saving pending changes (microsoft#4594) Remove usage of Number.NaN (microsoft#4615)

Markionium · 2018-04-20T18:45:26Z

I haven't found a good solution yet for globals in selectors that are not part of the same mergeStylesSet. Like the .contextualHost in the example below from office-ui-fabric-react/packages/office-ui-fabric-react/src/components/Persona/Persona.styles.ts

        selectors: {
          '.contextualHost': {
            display: 'none',
          },

          ':hover': {
            selectors: {
              '$primaryText': {
                color: palette.neutralDark,
              }
            }
          }
        }

Additionally this only takes care of global styles in the getStyles functions where we have the theme available. It does not work for all the instances where we just use css().

phkuo · 2018-04-21T01:02:26Z

packages/styling/src/styles/theme.ts

 import { loadTheme as legacyLoadTheme } from '@microsoft/load-themed-styles';

+const defaultThemeFlags: IThemeFlags = {
+  disableGlobalClassNames: false,


This should NOT be part of the theme, it's part of the global settings of the page. (In other words, it wouldn't make sense for this setting to change when you change the theme.)

@phkuo Assuming we mix together themes, it makes sense to me. I'm a little confused. In a portion of the ui, we expect the styling to not have global class names. It feels like the right approach to me.

What do you mean by "we mix together themes"?

The theme is independent of the CSS on the page. It doesn't know and it doesn't care about CSS. It should be portable between web and non-web platforms.

Let's say you have custom CSS on the page targeting global classes for a web part. It doesn't make sense that as you go from theme to theme, that web part randomly does or doesn't pick up your custom CSS. If you put custom global CSS on the page, you want to separately manage whether it applies or not independent of the theme. This setting should not be tied to the theme.

It should be treated like "theme" with @Customizable, where you can mark portions of the page with it. Then you can turn it on/off globally, or just set it for one dialog or panel.

If I understand you correctly, are you suggesting that:

We add a prop to every component like "disableGlobalClassNames".

We make that customizable.

We pipe this into the getStyles function props.

We use that prop, rather than theme.flags.disableGlobalClassNames

Because that seems really no better and just adds bloat. Is that somehow making devs' lives easier? Or the implementation cleaner?

When you say that you "hate adding more burden of knowledge on devs", what do you mean? What burden are you referring to? If I'm missing something, can you help come up with a clear alternative solution?

@dzearing
1.-3. seem like exactly the thing Customizer is for, no? Yes it does add bloat. I assumed any kind of global setting we'd want to be able to control it on a scoped basis as well. But I'm okay passing on it.

"Burden of knowledge" as in now every control has to have a const GlobalClassNames as well, on top of all the other stuff they already must do for getStyles(). I already have a two page document for myself on converting existing SCSS controls over to getStyles(), and I'm STILL adding to it. (I need to find time to polish and publish it.) I believe frameworks should remove boiler plate code, not add to it.
My proposed alternative is to put the logic in mergeStyles() (somewhere deep down in _processArgs()). If the passed in thing is a string, and it's not in our hash table, then throw it away.

I still STRONGLY believe it shouldn't be on the theme object. Example: you have a restaurant with a website. Part of the page is some list/table component that's the kids' menu. They're using global CSS to make the kids' menu Comic Sans, because it's a kids menu. Then they download some themes from online. Half the themes randomly make the kids' menu not respect the Comic Sans style, half of them do, with no rhyme or reason.
The theme should not contain settings.

I think we should not have global classes at all. They are a relic of the past and create a mess of pollution, bugs, and frustration.

But this is a compromise. We don't want to break backwards compatibility for the things we define.

If a caller passes in a global classname, that's fine. So no, this is not something we would "disable in mergeStyles".

So the "burden of knowledge" does not impact the consumer. It is 100% transparent to them. I think you're only concern is inside of Fabric components? But again, think of this as a flight that is by default on, but can be opted out of on demand.

Yes customizer can handle this. I'm not completely sure it makes sense to pollute the documented props with this though. THAT is a burden to the consumer.

@phkuo I kind of see your point and i had the same thought as you at first, that we could maybe do this in mergeStyles, but it also seems like the wrong place to do this and not really related to it's responsibilities.

As David says, we are doing this as a temporary solution and we would eventually like to remove global styles altogether, when that happens we can remove this flag from the theme again.

Additionally we are making people more aware when they are adding global styles by having them write the boiler plate (in the component.styles.ts files) might actually be a good thing as then they might reconsider if they actually need them in the first place ;)

Ah, so we'd do this for existing global class names, and then going forward we'd say no more global class names? In that case, you're right, it's no additional burden and there's no reason to pipe it through @customizer.

We want this on by default, right?

phkuo · 2018-04-21T01:05:15Z

@dzearing Can't we do this in mergeStyles/mergeStyleSets (not sure which, haven't looked at the code in a while), if it's of type string then discard it? Possibly with a check to verify it's not an autogenerated class name that we've stored somewhere. I hate that we're adding more and more burden of knowledge on devs.

dzearing · 2018-04-21T04:49:07Z

@phkuo I don't think we want to do this as a default for devs, so adding it to the type-safe ITheme interface seems pretty reasonable. It's far better than FabricConfig globals!

Markionium · 2018-04-21T11:00:49Z

@dzearing any ideas on how to resolve the thing i mentioned above?

#4635 (comment)

For example i had to revert this guy as i thought this would work, but screener will fail. (i'm used to it working from mergeStyleSets.)
bf0be8e#diff-866543420638c86a15e73d841b74e7a5

Any ideas? else i can do a bit more digging :) I suspect it has to do with how the result of getStyles is handled.

dzearing · 2018-04-23T01:31:02Z

I don't understand the purpose of

        selectors: {
          '.contextualHost': {
            display: 'none',
          },

@jordandrako ? (In persona.styles.ts)

dzearing · 2018-04-23T02:29:55Z

@Markionium yeah that's a confusing issue. "Reaching into" the private styling implementation of another component is really violating, as it can easily break, or be deprecated (I think in this specific example, I can't see a reason why we have that selector there.)

There are potentially cases where it makes sense, but I would rather do something like "import this classname constant from another file and reference it". If we did that then yes, we would still have a global class name, which really is used as an identifier more than a style representation.

Maybe I can catch you in the morning on monday and chat about it.

jordandrako · 2018-04-23T19:44:34Z

@dzearing It was in the sass file.

dzearing · 2018-04-24T05:54:26Z

@jordandrako I think it can be removed. There are no references to that in the repo that i see.

phkuo · 2018-04-25T00:15:49Z

packages/styling/src/interfaces/ITheme.ts

 import { IFontStyles } from './IFontStyles';
 import { ISemanticColors } from './ISemanticColors';

+export interface IThemeFlags {


I don't think we need a whole list of flags for the theme. This isn't something we want to be adding to. We can have it as a sibling to isInverted, and then just document it as... internal use only? Something that cautions people that it's for a very narrow use case and most people don't need to worry about it.

That sounds sensible to me. I've moved this onto the theme object and added a big comment ;)

.

dzearing · 2018-04-25T02:33:05Z

@Markionium Philip and I talked offline and he was misunderstanding this change. I think we're good now.

* master: (42 commits) Applying package updates. ProgressIndicator: Finish conversion to mergeStyles (microsoft#4595) Fix props validation for Breadcrumb (microsoft#4666) No unused vars part of ts (microsoft#4670) Picker/Autofill: fixes several minor bugs. (microsoft#4569) Fix Calendar component PREV/NEXT month, year, and "Go to today" handlers firing twice (microsoft#4662) Applying package updates. Merge styles order (microsoft#4664) Fabric component: revert class change and make it backwards compatible (microsoft#4671) Addressing Issue microsoft#3707 - OverflowSet: Add the ability to set aria-label (microsoft#4667) Fix input type for Tile ARIA label prop (microsoft#4668) Fix theme slots for DetailsList header colors (microsoft#4658) Applying package updates. Jolore/calendar updates (microsoft#4643) Remove wordWrap setting. (microsoft#4657) Pivot: convert to mergeStyles - part 1 (microsoft#4656) Use the `data-is-scrollable` attribute on the correct ScrollablePane div (microsoft#4602) Applying package updates. Remove unused iconClassName prop from Nav.types (microsoft#4634) Jest snapshots: classes in animations should autoexpand. (microsoft#4647) ...

…ium/office-ui-fabric-react into mapol/noGlobalClassNames * 'mapol/noGlobalClassNames' of https://github.com/Markionium/office-ui-fabric-react:

* master: Added className to Calendar component (microsoft#4680) Fix bug for OWA+Safari+popup (microsoft#4681) Add optional props for custom divider rendering (microsoft#4687) Update package.json Add isEqual and isNotEqual utility (microsoft#4603) Fix minor typos (microsoft#4683) Allow function to be passed to Customizer settings/scopedSettings (microsoft#4677)

Markionium added 4 commits April 19, 2018 19:29

Initial reference implementation

2a0af35

More components with disabled globals

dcf3398

Updates to how global classes are retrieved

79d13fc

Markionium requested review from ThomasMichon, cschleiden, joschect, micahgodbolt and phkuo as code owners April 20, 2018 18:21

Markionium added 3 commits April 20, 2018 20:25

Change flag name per suggestion

977732b

Remove unnecessary change

3465842

Add change files

6ede19f

Markionium requested a review from dzearing April 20, 2018 18:31

Markionium added 4 commits April 20, 2018 20:48

Use scoped className as parent instead of global string

c10cdda

Update test

f03fb83

Sub selectors not supported? :(

bf0be8e

Snapshot fix

1eaf1ae

phkuo previously requested changes Apr 21, 2018

View reviewed changes

dzearing approved these changes Apr 21, 2018

View reviewed changes

phkuo reviewed Apr 25, 2018

View reviewed changes

Merge branch 'master' into mapol/noGlobalClassNames

b678053

dzearing approved these changes Apr 25, 2018

View reviewed changes

Markionium added 4 commits April 26, 2018 16:46

Bump bundle size

276d5a0

Put global setting on theme instead of flags

dc5d7e8

Merge branch 'mapol/noGlobalClassNames' of https://github.com/Markion…

afc84a7

…ium/office-ui-fabric-react into mapol/noGlobalClassNames * 'mapol/noGlobalClassNames' of https://github.com/Markionium/office-ui-fabric-react:

Markionium mentioned this pull request Apr 26, 2018

Allow function to be passed to Customizer settings/scopedSettings #4677

Merged

2 tasks

Markionium added 2 commits April 26, 2018 20:50

Increase bundle size

e8d732d

Markionium force-pushed the mapol/noGlobalClassNames branch from 207d174 to 04df532 Compare April 27, 2018 10:04

Increase bundle size

da15818

Markionium force-pushed the mapol/noGlobalClassNames branch from 3e06c04 to da15818 Compare April 27, 2018 10:49

dzearing merged commit 6608006 into microsoft:master Apr 27, 2018

KevinTCoughlin mentioned this pull request May 7, 2018

Slider: Convert to mergeStyles (Part #1) #4796

Merged

1 task

microsoft locked as resolved and limited conversation to collaborators Aug 31, 2019

Flag on theme for no global class names #4635

Flag on theme for no global class names #4635

Uh oh!

Conversation

Markionium commented Apr 20, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pull request checklist

Description of changes

Focus areas to test

Uh oh!

Markionium commented Apr 20, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

phkuo Apr 21, 2018

Choose a reason for hiding this comment

Uh oh!

dzearing Apr 21, 2018

Choose a reason for hiding this comment

Uh oh!

phkuo Apr 21, 2018

Choose a reason for hiding this comment

Uh oh!

dzearing Apr 23, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

phkuo Apr 23, 2018

Choose a reason for hiding this comment

Uh oh!

dzearing Apr 24, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Markionium Apr 24, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

phkuo Apr 24, 2018

Choose a reason for hiding this comment

Uh oh!

phkuo commented Apr 21, 2018

Uh oh!

dzearing commented Apr 21, 2018

Uh oh!

Markionium commented Apr 21, 2018

Uh oh!

dzearing commented Apr 23, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dzearing commented Apr 23, 2018

Uh oh!

jordandrako commented Apr 23, 2018

Uh oh!

dzearing commented Apr 24, 2018

Uh oh!

phkuo Apr 25, 2018

Choose a reason for hiding this comment

Uh oh!

Markionium Apr 26, 2018

Choose a reason for hiding this comment

Uh oh!

dzearing commented Apr 25, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Markionium commented Apr 20, 2018 •

edited

Loading

Markionium commented Apr 20, 2018 •

edited

Loading

dzearing Apr 23, 2018 •

edited

Loading

dzearing Apr 24, 2018 •

edited

Loading

Markionium Apr 24, 2018 •

edited

Loading

dzearing commented Apr 23, 2018 •

edited

Loading