Skip to content

Part 2 of 3: DetailsList css-in-js conversion - DetailsHeader edition#5490

Merged
kenotron merged 10 commits intomicrosoft:masterfrom
kenotron:detailslist
Jul 10, 2018
Merged

Part 2 of 3: DetailsList css-in-js conversion - DetailsHeader edition#5490
kenotron merged 10 commits intomicrosoft:masterfrom
kenotron:detailslist

Conversation

@kenotron
Copy link
Copy Markdown
Contributor

@kenotron kenotron commented Jul 9, 2018

Pull request checklist

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

Description of changes

I'm breaking up the conversion one more part, so to keep the number of files changed small™️. This change includes the following:

  • DetailsHead
  • DetailsColumn

Focus areas to test

(optional)

Microsoft Reviewers: Open in CodeFlow

selectors: {
[`.${IsFocusVisibleClassName} &`]: {
opacity: 1
}
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Isn't this redundant with line 50? Based on the deleted code, I'm wondering if this should be outside the logic block starting at line 49...

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.

It is - I just had to remove this rule.

}
.ms-Fabric--isFocusVisible & {
opacity: 1;
}
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Should this appear somewhere else in the snapshot output?

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.

Nah - it's not needed apparently.

Copy link
Copy Markdown
Member

@JasonGore JasonGore Jul 10, 2018

Choose a reason for hiding this comment

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

But in some other snapshots it is reappearing in a different place...

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.

That's from getFocusStyles() that adds a ::after thing. It is quite a different selector altogether... This one is not necessary (been testing it all day)

...cellSizerFadeInStyles,
...{
boxShadow: '0 0 5px 0 0 rgba(0, 0, 0, 0.4)'
}
Copy link
Copy Markdown
Member

@JasonGore JasonGore Jul 10, 2018

Choose a reason for hiding this comment

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

Could this just be an array of objects instead of spreading two objects into one?

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.

Apparently not for styles listed inside a "selectors" part...

opacity: 1,
paddingLeft: 8,
selectors: {
'&$sortIcon': {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I noticed some places are like this while others on line 283 are just the name without quotes. Is there a reason for the difference?

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.

$foo$bar is legal identifier in JS whereas &$sortIcon isn't but it is a legal key for an object.

@kenotron kenotron merged commit 18d2b25 into microsoft:master Jul 10, 2018
@kenotron kenotron deleted the detailslist branch July 10, 2018 23:41
@microsoft microsoft locked as resolved and limited conversation to collaborators Aug 30, 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.

2 participants