-
Notifications
You must be signed in to change notification settings - Fork 2.9k
DatePicker MergeStyles step 2 - Converts scss to js styles #5362
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 6 commits
3813de8
4ef49bf
ff1cdd1
7867df2
4fb2b33
efb94d4
ed7ff40
e923dbd
c3fe7c4
f3209a5
79a4c25
1fa4871
fec4744
f22cb19
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": "office-ui-fabric-react", | ||
| "comment": "DatePicker MergeStyles step 2 - Converts scss to js styles", | ||
| "type": "minor" | ||
| } | ||
| ], | ||
| "packageName": "office-ui-fabric-react", | ||
| "email": "v-jescla@microsoft.com" | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,16 +1,24 @@ | ||
| import * as React from 'react'; | ||
| import { IDatePicker, IDatePickerProps, IDatePickerStrings } from './DatePicker.types'; | ||
| import { | ||
| IDatePicker, | ||
| IDatePickerProps, | ||
| IDatePickerStrings, | ||
| IDatePickerStyleProps, | ||
| IDatePickerStyles | ||
| } from './DatePicker.types'; | ||
| import { BaseComponent, KeyCodes, createRef, css, classNamesFunction } from '../../Utilities'; | ||
| import { Calendar, ICalendar, DayOfWeek } from '../../Calendar'; | ||
| import { FirstWeekOfYear } from '../../utilities/dateValues/DateValues'; | ||
| import { Callout } from '../../Callout'; | ||
| import { DirectionalHint } from '../../common/DirectionalHint'; | ||
| import { TextField, ITextField } from '../../TextField'; | ||
| import { BaseComponent, KeyCodes, css, createRef } from '../../Utilities'; | ||
| import { compareDates, compareDatePart } from '../../utilities/dateMath/DateMath'; | ||
| import * as stylesImport from './DatePicker.scss'; | ||
|
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'm surprised |
||
| import { FocusTrapZone } from '../../FocusTrapZone'; | ||
| const styles: any = stylesImport; | ||
|
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.
|
||
|
|
||
| const getClassNames = classNamesFunction<IDatePickerStyleProps, IDatePickerStyles>(); | ||
|
|
||
| export interface IDatePickerState { | ||
| selectedDate?: Date; | ||
| formattedDate?: string; | ||
|
|
@@ -46,7 +54,6 @@ const DEFAULT_STRINGS: IDatePickerStrings = { | |
| prevYearAriaLabel: 'Go to previous year', | ||
| nextYearAriaLabel: 'Go to next year' | ||
| }; | ||
|
|
||
|
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. Nitpicky as all hell I realize, but can you put this blank line before the back? Separating declarations like the class and this interface really help with scan-ability |
||
| export class DatePickerBase extends BaseComponent<IDatePickerProps, IDatePickerState> implements IDatePicker { | ||
| public static defaultProps: IDatePickerProps = { | ||
| allowTextInput: false, | ||
|
|
@@ -86,6 +93,11 @@ export class DatePickerBase extends BaseComponent<IDatePickerProps, IDatePickerS | |
| private _textField = createRef<ITextField>(); | ||
| private _preventFocusOpeningPicker: boolean; | ||
|
|
||
| private _classNames = getClassNames(this.props.styles, { | ||
| theme: this.props.theme!, | ||
| className: this.props.className | ||
| }); | ||
|
|
||
| constructor(props: IDatePickerProps) { | ||
| super(props); | ||
| this.state = this._getDefaultState(); | ||
|
|
@@ -153,19 +165,19 @@ export class DatePickerBase extends BaseComponent<IDatePickerProps, IDatePickerS | |
| placeholder, | ||
| allowTextInput, | ||
| borderless, | ||
| className, | ||
| minDate, | ||
| maxDate, | ||
| calendarProps | ||
| } = this.props; | ||
| const { isDatePickerShown, formattedDate, selectedDate, errorMessage } = this.state; | ||
| const { _classNames } = this; | ||
|
|
||
| return ( | ||
| <div className={css('ms-DatePicker', styles.root, isDatePickerShown && 'is-open', className)}> | ||
| <div className={_classNames.root}> | ||
| <div ref={this._datePickerDiv}> | ||
| <TextField | ||
| label={label} | ||
| className={styles.textField} | ||
| className={_classNames.textField} | ||
| ariaLabel={ariaLabel} | ||
| aria-haspopup="true" | ||
| aria-expanded={isDatePickerShown} | ||
|
|
@@ -184,8 +196,7 @@ export class DatePickerBase extends BaseComponent<IDatePickerProps, IDatePickerS | |
| onClick: this._onIconClick, | ||
| className: css( | ||
|
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. This concatination logic should be in the styles.ts file. There |
||
| disabled && styles.msDatePickerDisabled, | ||
| label ? 'ms-DatePicker-event--with-label' : 'ms-DatePicker-event--without-label', | ||
| label ? styles.eventWithLabel : styles.eventWithoutLabel | ||
| label ? _classNames.eventWithLabel : _classNames.eventWithoutLabel | ||
| ) | ||
| }} | ||
| readOnly={!allowTextInput} | ||
|
|
||
This file was deleted.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,17 +1,75 @@ | ||
| import { IDatePickerStyleProps, IDatePickerStyles } from './DatePicker.types'; | ||
| import { IStyle, normalize, getGlobalClassNames } from '../../Styling'; | ||
|
|
||
| export const getStyles = (props: IDatePickerStyleProps): IDatePickerStyles => { | ||
| const { className } = props; | ||
| const GlobalClassNames = { | ||
| root: 'ms-DatePicker', | ||
|
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. 👍 |
||
| textField: 'ms-TextField', | ||
|
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. duplicate - 'ms-TextField' comes from the TextField component root, so when they combine you render two 'ms-TextField' s - the scss implementation didn't have a class for this element just 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. Yeah, this className should be removed |
||
| eventWithLabel: 'ms-DatePicker-event--with-label', | ||
| eventWithoutLabel: 'ms-DatePicker-event--without-label' | ||
| }; | ||
|
|
||
| export const styles = (props: IDatePickerStyleProps): IDatePickerStyles => { | ||
| const { className, theme } = props; | ||
| const { palette, fonts } = theme; | ||
| const classNames = getGlobalClassNames(GlobalClassNames, theme); | ||
|
|
||
| const DatePickerEvent: IStyle = { | ||
| color: palette.neutralSecondary, | ||
| fontSize: fonts.medium.fontSize, | ||
| lineHeight: '18px', | ||
| pointerEvents: 'none', | ||
| position: 'absolute', | ||
| right: '9px' | ||
| }; | ||
|
|
||
| return { | ||
| root: [ | ||
| 'ms-DatePicker', | ||
| root: [classNames.root, normalize, {}, className], | ||
| textField: [ | ||
| classNames.textField, | ||
| { | ||
| // Insert css properties | ||
| position: 'relative', | ||
| selectors: { | ||
| '& input[readonly]': { | ||
| cursor: 'pointer' | ||
| }, | ||
| input: { | ||
| selectors: { | ||
| '::-ms-clear': { | ||
| display: 'none' | ||
| } | ||
| } | ||
| } | ||
| } | ||
| }, | ||
| className | ||
| ], | ||
| eventWithLabel: [ | ||
| classNames.eventWithLabel, | ||
| DatePickerEvent, | ||
| { | ||
| bottom: '5px', | ||
| selectors: { | ||
| ':not(.msDatePickerDisabled)': { | ||
| pointerEvents: 'initial', | ||
| cursor: 'pointer' | ||
| } | ||
| } | ||
| }, | ||
| className | ||
|
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. Shouldn't className only be on root? I don't see it mixed in with other class names in old code
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. Yes, the standard className prop is a custom class name just for the root, not all elements. |
||
| ], | ||
| eventWithoutLabel: [ | ||
| classNames.eventWithoutLabel, | ||
| DatePickerEvent, | ||
| { | ||
| top: '7px', | ||
| selectors: { | ||
| ':not(.msDatePickerDisabled)': { | ||
| pointerEvents: 'initial', | ||
| cursor: 'pointer' | ||
| } | ||
| } | ||
| }, | ||
| className | ||
| ] | ||
|
|
||
| // Insert className styles | ||
| }; | ||
| }; | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,14 +1,14 @@ | ||
| import { styled } from '../../Utilities'; | ||
| import { IDatePickerProps, IDatePickerStyleProps, IDatePickerStyles } from './DatePicker.types'; | ||
| import { DatePickerBase } from './DatePicker.base'; | ||
| import { getStyles } from './DatePicker.styles'; | ||
| import { styles } from './DatePicker.styles'; | ||
|
|
||
| /** | ||
| * DatePicker description | ||
| */ | ||
| export const DatePicker = styled<IDatePickerProps, IDatePickerStyleProps, IDatePickerStyles>( | ||
| DatePickerBase, | ||
| getStyles, | ||
| styles, | ||
| undefined, | ||
| { scope: 'DatePicker' } | ||
| ); |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2,7 +2,7 @@ | |
|
|
||
| exports[`DatePicker renders default DatePicker correctly 1`] = ` | ||
| <div | ||
| className="ms-DatePicker" | ||
| className={undefined} | ||
|
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. this should not be undefined
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. Per our Teams chat, |
||
| > | ||
| <div> | ||
| <div | ||
|
|
@@ -39,7 +39,7 @@ exports[`DatePicker renders default DatePicker correctly 1`] = ` | |
| <i | ||
| aria-hidden={true} | ||
| className= | ||
| ms-DatePicker-event--without-label | ||
|
|
||
|
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. Hmm, I feel like this className should still be here but I don't see any obvious issues with the code |
||
| { | ||
| -moz-osx-font-smoothing: grayscale; | ||
| -webkit-font-smoothing: antialiased; | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -8,11 +8,35 @@ exports[`Component Examples renders DatePicker.Bounded.Example.tsx correctly 1`] | |
| When date boundaries are set (via minDate and maxDate props) the DatePicker will not allow out-of-bounds dates to be picked or entered. In this example, the allowed dates are 2018-5-13-2018-5-13 | ||
| </p> | ||
| <div | ||
| className="ms-DatePicker" | ||
| className= | ||
| ms-DatePicker | ||
| { | ||
| box-shadow: none; | ||
| box-sizing: border-box; | ||
| margin-bottom: 0px; | ||
| margin-left: 0px; | ||
| margin-right: 0px; | ||
| margin-top: 0px; | ||
| padding-bottom: 0px; | ||
| padding-left: 0px; | ||
| padding-right: 0px; | ||
| padding-top: 0px; | ||
| } | ||
| > | ||
| <div> | ||
| <div | ||
| className="ms-TextField" | ||
| className= | ||
| ms-TextField | ||
| ms-TextField | ||
|
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's odd, I wonder why
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. coming from TextField root, see my comment in DatePicker.styles.ts for the fix |
||
| { | ||
| position: relative; | ||
| } | ||
| & input[readonly] { | ||
| cursor: pointer; | ||
| } | ||
| & input::-ms-clear { | ||
| display: none; | ||
| } | ||
| > | ||
| <div | ||
| className="ms-TextField-wrapper" | ||
|
|
@@ -49,11 +73,22 @@ exports[`Component Examples renders DatePicker.Bounded.Example.tsx correctly 1`] | |
| { | ||
| -moz-osx-font-smoothing: grayscale; | ||
| -webkit-font-smoothing: antialiased; | ||
| color: #666666; | ||
| display: inline-block; | ||
| font-family: "FabricMDL2Icons"; | ||
| font-size: 14px; | ||
| font-style: normal; | ||
| font-weight: normal; | ||
| line-height: 18px; | ||
| pointer-events: none; | ||
| position: absolute; | ||
| right: 9px; | ||
| speak: none; | ||
| top: 7px; | ||
| } | ||
| &:not(.msDatePickerDisabled) { | ||
| cursor: pointer; | ||
| pointer-events: initial; | ||
| } | ||
| data-icon-name="Calendar" | ||
| onClick={[Function]} | ||
|
|
||
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.
cssimport should be removed - there's a completely redundant call on line 213 that shouldn't have been there in the first place (its 'concatenating' just one class with nothing else