-
Notifications
You must be signed in to change notification settings - Fork 2.9k
Convert Overlay to mergeStyles #3978
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
Changes from 16 commits
1a437dd
5e96f55
c194e12
b9ed951
8bab64c
bbd14ae
f33d62d
0d95ef2
80f313c
d3936dd
44bd6bd
5472fa3
900b7e7
9318d3f
fe611c9
0b38726
24bea97
9733340
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,11 @@ | ||
| { | ||
| "changes": [ | ||
| { | ||
| "packageName": "@uifabric/styling", | ||
| "comment": "Add whiteTranslucent40 to palette", | ||
| "type": "patch" | ||
| } | ||
| ], | ||
| "packageName": "@uifabric/styling", | ||
| "email": "v-jojanz@microsoft.com" | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,11 @@ | ||
| { | ||
| "changes": [ | ||
| { | ||
| "packageName": "office-ui-fabric-react", | ||
| "comment": "Convert overlay and examples to mergeStyles", | ||
| "type": "patch" | ||
| } | ||
| ], | ||
| "packageName": "office-ui-fabric-react", | ||
| "email": "v-jojanz@microsoft.com" | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,50 @@ | ||
| import * as React from 'react'; | ||
| import { | ||
| BaseComponent, | ||
| customizable, | ||
| classNamesFunction, | ||
| getNativeProps, | ||
| divProperties, | ||
| enableBodyScroll, | ||
| disableBodyScroll | ||
| } from '../../Utilities'; | ||
| import { | ||
| IOverlayProps, | ||
| IOverlayStyleProps, | ||
| IOverlayStyles, | ||
| } from './Overlay.types'; | ||
|
|
||
| const getClassNames = classNamesFunction<IOverlayStyleProps, IOverlayStyles>(); | ||
|
|
||
| @customizable('Overlay', ['theme']) | ||
| export class OverlayBase extends BaseComponent<IOverlayProps, {}> { | ||
|
|
||
| public componentDidMount() { | ||
| disableBodyScroll(); | ||
| } | ||
|
|
||
| public componentWillUnmount() { | ||
| enableBodyScroll(); | ||
| } | ||
|
|
||
| public render() { | ||
| const { | ||
| isDarkThemed, | ||
| className, | ||
| theme, | ||
| getStyles | ||
| } = this.props; | ||
|
|
||
| const divProps = getNativeProps(this.props, divProperties); | ||
|
|
||
| const classNames = getClassNames(getStyles!, { | ||
| theme: theme!, | ||
| className, | ||
| isDarkThemed, | ||
| }); | ||
|
|
||
| return ( | ||
| <div { ...divProps } className={ classNames.root } /> | ||
| ); | ||
| } | ||
| } |
This file was deleted.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,52 @@ | ||
| import { IOverlayStyleProps, IOverlayStyles } from './Overlay.types'; | ||
| import { | ||
| IStyle, | ||
| ITheme, | ||
| HighContrastSelector, | ||
| } from '../../Styling'; | ||
|
|
||
| export const getStyles = ( | ||
| props: IOverlayStyleProps | ||
| ): IOverlayStyles => { | ||
| const { | ||
| className, | ||
| theme, | ||
| isNone, | ||
| isDarkThemed, | ||
| } = props; | ||
|
|
||
| const { palette, semanticColors } = theme; | ||
|
|
||
| return ({ | ||
| root: [ | ||
| 'ms-Overlay', | ||
| { | ||
| backgroundColor: palette.whiteTranslucent40, | ||
| top: 0, | ||
| right: 0, | ||
| bottom: 0, | ||
| left: 0, | ||
| position: 'absolute', | ||
|
|
||
| selectors: { | ||
| [HighContrastSelector]: { | ||
| border: '1px solid WindowText', | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is WindowText a valid choice here? This may be High Contrast Mode specific, but I just imagine it would be better to use a theme color.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yup, it's HighContrast specific. |
||
| } | ||
| } | ||
| }, | ||
|
|
||
| isNone && { | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Process question: From what I can tell this was never actually applied before (just an unused SCSS class), and you went ahead and applied that as an optional prop. Do we want to do this, or assume abandoned functionality was abandoned for a reason. Like in this case does it serve a purpose or is it just a superfluous prop taking up bit space.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good question. @battletoilet |
||
| visibility: 'hidden', | ||
| }, | ||
|
|
||
| isDarkThemed && [ | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could we rename this to
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'll have to deprecate it but sure.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Even if we don't change the component's prop, this style prop could be changed for now right?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ah, yeah. |
||
| 'ms-Overlay--dark', | ||
| { | ||
| backgroundColor: palette.blackTranslucent40, | ||
| } | ||
| ], | ||
|
|
||
| className | ||
| ], | ||
| }); | ||
| }; | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,41 +1,13 @@ | ||
| import * as React from 'react'; | ||
| import { styled } from '../../Utilities'; | ||
| import { | ||
| BaseComponent, | ||
| css, | ||
| getNativeProps, | ||
| divProperties, | ||
| enableBodyScroll, | ||
| disableBodyScroll | ||
| } from '../../Utilities'; | ||
| import { IOverlayProps } from './Overlay.types'; | ||
|
|
||
| import * as stylesImport from './Overlay.scss'; | ||
| const styles: any = stylesImport; | ||
|
|
||
| export class Overlay extends BaseComponent<IOverlayProps, {}> { | ||
|
|
||
| public componentDidMount() { | ||
| disableBodyScroll(); | ||
| } | ||
|
|
||
| public componentWillUnmount() { | ||
| enableBodyScroll(); | ||
| } | ||
|
|
||
| public render() { | ||
| let { isDarkThemed, className } = this.props; | ||
| let divProps = getNativeProps(this.props, divProperties); | ||
|
|
||
| let modifiedClassName = css( | ||
| 'ms-Overlay', | ||
| styles.root, | ||
| className, | ||
| { | ||
| ['ms-Overlay--dark ' + styles.rootIsDark]: isDarkThemed, | ||
| }); | ||
|
|
||
| return ( | ||
| <div { ...divProps } className={ modifiedClassName } /> | ||
| ); | ||
| } | ||
| } | ||
| IOverlayProps, | ||
| IOverlayStyleProps, | ||
| IOverlayStyles | ||
| } from './Overlay.types'; | ||
| import { OverlayBase } from './Overlay.base'; | ||
| import { getStyles } from './Overlay.styles'; | ||
|
|
||
| export const Overlay = styled<IOverlayProps, IOverlayStyleProps, IOverlayStyles>( | ||
| OverlayBase, | ||
| getStyles | ||
| ); |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,19 +1,68 @@ | ||
| import * as React from 'react'; | ||
| import { OverlayBase } from './Overlay.base'; | ||
| import { IStyle, ITheme } from '../../Styling'; | ||
| import { IStyleFunction } from '../../Utilities'; | ||
|
|
||
| export interface IOverlay { | ||
|
|
||
| } | ||
|
|
||
| export interface IOverlayProps extends React.HTMLAttributes<HTMLElement> { | ||
| /** | ||
| * Optional callback to access the IOverlay interface. Use this instead of ref for accessing | ||
| * the public methods and properties of the component. | ||
| * Gets the component ref. | ||
| */ | ||
| componentRef?: (component: IOverlay) => void; | ||
| componentRef?: (component: IOverlayProps) => void; | ||
|
|
||
| /** | ||
| * Call to provide customized styling that will layer on top of the variant rules | ||
| */ | ||
| getStyles?: IStyleFunction<IOverlayStyleProps, IOverlayStyles>; | ||
|
|
||
| /** | ||
| * Theme provided by HOC. | ||
| */ | ||
| theme?: ITheme; | ||
|
|
||
| /** | ||
| * Additional css class to apply to the Overlay | ||
| * @defaultvalue undefined | ||
| */ | ||
| className?: string; | ||
|
|
||
| /** | ||
| * Whether to use the dark-themed overlay. | ||
| * @defaultvalue false | ||
| */ | ||
| isDarkThemed?: boolean; | ||
|
|
||
| onClick?: () => void; | ||
| } | ||
|
|
||
| export interface IOverlayStyleProps { | ||
| /** | ||
| * Accept theme prop. | ||
| */ | ||
| theme: ITheme; | ||
|
|
||
| /** | ||
| * Accept custom classNames | ||
| */ | ||
| className?: string; | ||
|
|
||
| /** | ||
| * Is overlay visible | ||
| */ | ||
| isNone?: boolean; | ||
|
|
||
| /** | ||
| * Is overlay dark themed | ||
| */ | ||
| isDarkThemed?: boolean; | ||
| } | ||
|
|
||
| export interface IOverlayStyles { | ||
| /** | ||
| * Style for the root element. | ||
| */ | ||
| root?: IStyle; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. don't we want all classNames to be required? @dzearing @mikewheaton
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. that way we can insure, inside getStyles, that we're making the entire thing. and then other consumers customizing it can just say Partial if they're merging with the existing one
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In this case, having them required prevents customizing the styles via the
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could you point me to an example of Partial styles?
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I would like for us to have a way to insure any and all classNames defined in IFooStyles gets defined in the default implementation, while allowing other implementations to use a Partial<> to only have to override a subset.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Excuse me, the Component I was thinking of that uses customStyles is Checkbox, which I have another PR out for. I'll keep Partial in mind. |
||
| } | ||
This file was deleted.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,23 @@ | ||
| import { IStyle } from '../../../Styling'; | ||
|
|
||
| export interface IOverlayExampleStyles { | ||
| root: IStyle; | ||
| } | ||
|
|
||
| export const getStyles = (): IOverlayExampleStyles => { | ||
|
|
||
| return ({ | ||
| root: [ | ||
| 'OverlayExample-content', | ||
| { | ||
| background: 'blue', | ||
| bottom: '0', | ||
| color: 'white', | ||
| left: '0', | ||
| padding: '10px', | ||
| position: 'absolute', | ||
| right: '0', | ||
| } | ||
| ] | ||
| }); | ||
| }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could this be renamed for clarity? Maybe reverse it and call it
isVisible?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed, it's not used at the moment so that should be remedied.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we remove it entirely then?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had the same question but forgot to follow up on it. @battletoilet - thoughts?