fluentui
fluentui copied to clipboard
microsoft
[Bug]: DetailsList and GroupedList do not calculate group nesting level correctly - misaligned
Library
React / v8 (@fluentui/react)
System Info
System:
OS: Windows 10 10.0.22000
CPU: (8) x64 Intel(R) Core(TM) i7-1065G7 CPU @ 1.30GHz
Memory: 5.87 GB / 15.59 GB
Browsers:
Edge: Spartan (44.22000.120.0), Chromium (101.0.1210.47)
Internet Explorer: 11.0.22000.120
Are you reporting Accessibility issue?
No response
Reproduction
https://codepen.io/TonyH/pen/oNEwBJW
Bug Description
Actual Behavior
DetailsList ( and Grouped ) are not aligned as well as an unnecessary horizontal scroolbar for DetailsList.
Image from https://codepen.io/TonyH/pen/oNEwBJW
Expected Behavior
They are aligned. The scrollbar is not present.
Image from https://codepen.io/TonyH/pen/oNEwBJW
One of the causes of the issue is the calculation of the group nesting depth. Bizarrely it does not walk the full tree to arrive at this value. This occurs in both the DetailsList and the GroupedList. It causes the header spacing to be incorrect and the horizontal scrollbars for the DetailsList. The other is that the GroupedListSection will call onRenderCell with the nesting level of the group and not of the tree of groups. This results in incorrect spacing for the row.
Note that the issue has not presented itself in examples ( storybook / website ) as there is a consistent depth in the nested groups and the algorithm happens to be correct.
Also note that there is currently not a single test with a group with children.
I have a draft fork that has made the following changes.
Changed the DetailsList group example to be jagged that would demonstrate the issue - similar to the reproduction
Corrected the logic ( certainly for DetailsList - more on this in a moment) Have added a new prop for DetailsRow - totalNestingDepth. The reason is that the groupNestingDepth can continue to be used for aria and the totalNestingDepth for spacing. The "RowFields" receives this corrected nesting depth in columnStartIndex which is unused in the default DetailsRowFields
What remains to be fixed is the calling of the onRenderCell prop of the GroupedList. Currently this receives the depth of the current group and not the depth of the full tree.
Preferences for parameter position for additional totalNestingDepth ?
onRenderCell: (nestingDepth?: number, item?: any, index?: number, // add here) => React.ReactNode
The remaining change ( hopefully - perhaps aria too ) would be for the GroupedList to pass on its own onRenderCell that when called will call the props function with the totalNestingDepth.
I have not been able to complete my pr. My tests pass but yarn buildci fails with errors that appear unrelated to my changes
Logs
No response
Requested priority
High
Products/sites affected
No response
Are you willing to submit a PR to fix?
yes
Validations
- [X] Check that there isn't already an issue that reports the same bug to avoid creating a duplicate.
- [X] The provided reproduction is a minimal reproducible example of the bug.
@tonyhallett - thank you for filing this issue with. To set expectations, we’re likely not going to get to it for a while as we’re currently focused on larger coherence work items. However, I see you are willing contribute to this issue, that is great news! If you have a solution at hand and would like to contribute towards it, please, share your PR for us and the team will review it.
This issue has been automatically marked as stale because it has marked as requiring author feedback but has not had any activity for 4 days. It will be closed if no further activity occurs within 3 days of this comment. Thank you for your contributions to Fluent UI!
Why is this requiring author feedback ? https://github.com/microsoft/fluentui/issues/23135#issuecomment-1164250079
failed with yarn buildci which appeared to be unrelated to the changes that they had made.
@tonyhallett Thanks for the detailed issue. Lots of details are always helpful, especially when considering changes to a large component like DetailsList.
For the buildci issue it looks like you're running that on your local machine and I'm not certain that command reliably works locally. Typically building just the project you're affecting is enough to verify the build will pass (not always of course but I don't think you'll have many issues in this case). For @fluentui/react the build command is run from the repo root:
yarn workspace @fluentui/react build
Would you mind opening a draft PR against the Fluent repo? I'd like to be able to more easily see your code changes. There are also some other cases that we'll probably need to clean up -- your change makes the column header (Name, Color) misaligned.
In fact, looking at both the before and after screenshots you posted there seems to be some misalignment in both to my eyes. For example, "Primary Colors" is not really aligned with the column headers in either example. I need to talk with some folks internally to understand what was intended here.
I'll follow up when I have more information about original nesting intent.
@spmonahan
I was using buildci as is it is documented https://github.com/microsoft/fluentui/wiki/Build-Commands#before-check-in I will just build react.
your change makes the column header (Name, Color) misaligned.
In the original the Name and Color columns are aligned to the items in the first grouping level but the single item in the second grouping level is misaligned. In the sandbox "fix" that I made the Name and Color columns are aligned to the items in all columns.
In fact, looking at both the before and after screenshots you posted there seems to be some misalignment in both to my eyes. For example, "Primary Colors" is not really aligned with the column headers in either example.
I agree. I believe that all of the group headers should be aligned too. For the DetailsList - Grouped demo the headers do align with the first column. This was to be done in the pr.
I have now added this code - inserting <GroupSpacer> elements with desired count by providing onRenderTitle in the IGroupedListProps.
I will create draft pull request in a short while. I will not add further tests for the change to the group header alignment until it is agreed that the intent is as we expect it to be.
@spmonahan Pr is failing on API extractor. I committed the changed file as per docs. Perhaps you can help.
Because this issue has not had activity for over 180 days, we're automatically closing it for house-keeping purposes.
Still require assistance? Please, create a new issue with up-to date details.