DetailsList: Remove unnecessary styles #13540
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Perf AnalysisNo significant results to display. All results
Perf Analysis (Fluent)Perf comparison
Perf tests with no regressions
|
Asset size changes
Baseline commit: 52da3b627c30777bfc11db0b87f68494c6a6a86a (build) |
ThomasMichon
approved these changes
Jun 10, 2020
|
Hello @phkuo! Because this pull request has the p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (
|
|
Just being cautious here, but any time we usually touch styling that has the potential to break others - especially if are changing/removing behavior. Do folks have an idea of how impactful these changes are? |
|
@paulgildea Yes I understand the concern. We do have several visual regression tests in place here in FluentUI run as part of the PR. I've been playing around with these changes in SharePoint in different lists to verify that there won't be regressions for the big big lists there. I've also consulted Thomas, who has more experience than me in this feature area. He's actually piloted removing the pixel-based in-width from the header already, and can confirm that it works as expected. Regarding the removal of the background, that'd actually be buggy if anyone were taking advantage of it. |
dzearing
reviewed
Jun 11, 2020
| @@ -218,7 +217,6 @@ export class DetailsHeaderBase extends React.Component<IDetailsHeaderBaseProps, | |||
| ref={this._onRootRef} | |||
| onMouseMove={this._onRootMouseMove} | |||
| data-automationid="DetailsHeader" | |||
| style={{ minWidth: rowWidth }} | |||
| direction={FocusZoneDirection.horizontal} | |||
There was a problem hiding this comment.
I think this will shrink the header to a smaller width than rows.
There was a problem hiding this comment.
it does not in my testing, because each column has a width set anyways
i've been testing with column resize as well
i'm honestly not sure why rowWidth exists in the first place if each field's width needs to be calculated individually anyways
@ThomasMichon any thoughts?
KevinTCoughlin
approved these changes
Jun 11, 2020
|
🎉 Handy links: |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Pull request checklist
$ yarn changeDescription of changes
Removes some styles that do nothing for the list. See attached workitem.