Skip to content

Dialog: Converting to mergeStyles part 2 - style conversion#4160

Merged
jordandrako merged 30 commits intomicrosoft:masterfrom
jordandrako:mergeStyles/Dialog-round2
Mar 8, 2018
Merged

Dialog: Converting to mergeStyles part 2 - style conversion#4160
jordandrako merged 30 commits intomicrosoft:masterfrom
jordandrako:mergeStyles/Dialog-round2

Conversation

@jordandrako
Copy link
Copy Markdown
Contributor

Pull request checklist

  • Include a change request file using $ npm run change

Description of changes

  • Finish converting Dialog and it's sub components to mergeStyles
  • Add isMultiline prop to DialogContent (accessed by setting dialogContentProps={{ isMultiline: true }})

Focus areas to test

I couldn't find a good way to convert some of the styles that were nested under the class .isMultiline, which seems to be from other components like DetailsList, but not sure if this class was applied in the first place with the way it was written.

//= Modifier: Multiline button dialog
//
.isMultiline {
  .title {
    font-size: $ms-font-size-xxl;
  }

  .inner {
    padding: 0 20px 20px;
  }
}

Copy link
Copy Markdown
Collaborator

@oengusmacinog-zz oengusmacinog-zz left a comment

Choose a reason for hiding this comment

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

Few notes. I feel like the bigger the style conversion the harder it will be to catch bits of logic that fell through the cracks when translated from the base.ts file to the styles.ts file - but screener should catch the big stuff there

return ({
content: [
'ms-DialogContent',
type === DialogType.largeHeader && [
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

type === DialogType.largeHeader && 'ms-Dialog-lgHeader' is a shorter way to do these ones that are just class no style

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Ah, yeah, I wrote them like that before I copied styles over thinking they'd have styles, thanks.

import * as stylesImport from './Dialog.scss';
const styles: any = stylesImport;
const getClassNames = classNamesFunction<IDialogStyleProps, IDialogStyles>();
// import * as stylesImport from './Dialog.scss';
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

These can be removed

} = props;

const { palette, semanticColors } = theme;
const dialogDefaultMinWidth = '288px';
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I think it might be better to make these a style prop you can pass the default in from base

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Agreed.

@jordandrako jordandrako closed this Mar 2, 2018
@jordandrako jordandrako reopened this Mar 2, 2018
} = props;

const { palette, fonts } = theme;
const dialogLgHeaderBackgroundColor = palette.themePrimary;
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

dialogLgHeaderBackgroundColor is redundant as its only used once. just use palette.themePrimary where it's used in the styles and nix the extra variable.

@microsoft microsoft deleted a comment from jordandrako Mar 5, 2018

return ({
content: [
'ms-DialogContent',
Copy link
Copy Markdown
Collaborator

@oengusmacinog-zz oengusmacinog-zz Mar 5, 2018

Choose a reason for hiding this comment

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

I see that this wasn't here before, but I say leave the class 'ms-DialogContent' in. Its an easy semantic way to find the root of this sub/sibling component in the final HTML and that could be useful - at least with any root items that have className passed to them.

const { getStyles, theme } = this.props;

this._classNames = getClassNames(getStyles!, {
theme: theme!,
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I think if it is a Component class root, it should have a className style prop. With normal usage, you wont really pass anything to it, but DialogFooter is in the index.ts so a dev can grab use it in a custom way and if they do they should be able to pass use the className prop.

Copy link
Copy Markdown
Collaborator

@oengusmacinog-zz oengusmacinog-zz left a comment

Choose a reason for hiding this comment

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

I think we're good!

@jordandrako
Copy link
Copy Markdown
Contributor Author

@dzearing This uses a static defaultProps, is the theme decorator going to cause problems?

@jordandrako jordandrako merged commit b410f6b into microsoft:master Mar 8, 2018
@microsoft microsoft locked as resolved and limited conversation to collaborators Aug 31, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants