Part 3 of 3: DetailsList CSS-in-JS conversion #5514
Conversation
|
@JasonGore The last one! This one is significantly smaller than the rest of the DL conversion PRs |
| "changes": [ | ||
| { | ||
| "packageName": "office-ui-fabric-react", | ||
| "comment": "Remaining CSS-in-JS conversion of DetailsList", |
There was a problem hiding this comment.
DetailsList: Complete CSS-in-JS conversion.
| @@ -7,5 +7,7 @@ export { IDetailsHeaderProps }; | |||
|
|
|||
| export const DetailsHeader = styled<IDetailsHeaderProps, IDetailsHeaderStyleProps, IDetailsHeaderStyles>( | |||
| DetailsHeaderBase, | |||
| getStyles | |||
| getStyles, | |||
There was a problem hiding this comment.
Rename to DetailsHeader.ts (same thing for the other files like this)
| @@ -1,968 +1,15 @@ | |||
| import * as React from 'react'; | |||
| import * as stylesImport from './DetailsList.scss'; | |||
| import { styled } from '../../Utilities'; | |||
There was a problem hiding this comment.
If you did a rename of DetalisList.tsx -> DetailsList.base.tsx in a commit,, then create a brand new DetailsList.ts (not tsx) with the styled export, the line history gets preserved and you don't show up on gitblame for the entire file :p
| export interface IDetailsListProps extends React.Props<DetailsListBase>, IWithViewportProps { | ||
| theme?: ITheme; | ||
|
|
||
| styles?: IStyleFunctionOrObject<IDetailsListStyleProps, IDetailsListStyles>; |
There was a problem hiding this comment.
Can we please document both styles and themes? (I'm sure you can find examples of components which do not, but most do - and we really ought to be documenting)
|
|
||
| export type IDetailsListStyleProps = Required<Pick<IDetailsListProps, 'theme'>> & | ||
| Pick<IDetailsListProps, 'className'> & { | ||
| isHorizontalConstrained?: boolean; |
There was a problem hiding this comment.
We really should be documenting these as well, if you think about the purpose these props serve and from the POV of someone writing a style function for DetailsList
There was a problem hiding this comment.
added more docs to these
|
|
||
| export interface IDetailsListStyles { | ||
| root: IStyle; | ||
| focusZone: IStyle; |
There was a problem hiding this comment.
And arguably these too (again there are various examples of components which do complete documentation). But minimally the styleprops.
(Look at OverflowSet.types.ts for example...)
There was a problem hiding this comment.
I'm documenting the styleprops for now - DL subcomponents are not expected to be used directly by others. They're actually kind of a distraction. There are only two styles here and they seem to be pretty self-explanatory.
| <ShimmeredDetailsList | ||
| items={mockItems(5)} | ||
| // tslint:disable-next-line:jsx-no-lambda | ||
| onRenderRow={() => null} |
There was a problem hiding this comment.
Why not just use jest.fn() in places like this?
There was a problem hiding this comment.
I just cloned this from DL's test... didn't seem to be a big deal to do a jest.fn() or have props be passed in, in this case
There was a problem hiding this comment.
Agree it's not a big deal - but you can avoid the tslint disable this way :) Again, no big deal.
Pull request checklist
$ npm run changeDescription of changes
This is a relatively small final checkin to convert the last bits of detailslist to use mergestyles.
Focus areas to test
(optional)
Microsoft Reviewers: Open in CodeFlow