Tidy up results screen statistic item flow #23718
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
There were no usages of more than one column being provided per row, so it seemed like unnecessarily complexity. I'm currently trying to reduce complexity so we can improve the layout of the results screen, which currently has up to three levels of nested `GridContainer`s. Of note, I can't add backwards compatibility because the method signature has not changed in `Ruleset` (only the return type). If we do want to keep compatibility with other rulesets, we could designate a new name for the updated method.
`dimensions` would always receive exactly one item, so might as well
inline it.
And yes, at this point the grid container is mostly a glorified
`FillFlowContainer { Direction = FlowDirection.Vertical }`, but I am not
touching that in this pull pending further decisions with respect to
direction.
|
Can agree with all of this. I'm pretty sure I voiced this exact same opinion about the results screen at some point; making a good layout that will work on all resolutions is basically impossible with the current setup and layout. Given the fact that the next release has been branched off, is it OK to tick the checkbox in the OP and merge, or do you want a second opinion from someone else? |
bdach
approved these changes
Jun 3, 2023
|
Given that the checkbox has been checked (and it wasn't me who did it), I'll take it that it is fine to move forward with this. |
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.
Two preliminary changes here that I wanted to make before attempting to fix up the actual screen layout.
985604f Return
StatisticItems rather thanStatisticRows from rulesetThere were no usages of more than one column being provided per row, so it seemed like unnecessarily complexity. I'm currently trying to reduce complexity so we can improve the layout of the results screen, which currently has up to three levels of nested
GridContainers.Of note, I can't add backwards compatibility because the method signature has not changed in
Ruleset(only the return type). If we do want to keep compatibility with other rulesets, we could designate a new name for the updated method.dc595b8 Remove unused
Dimensionspecification fromStatisticItemDoesn't look to have been ever used. Going forward the grid container which gets this assigned to it is likely to disappear. If the goal was to allow elements to specify their width, I am fine with that but we should add it back in a more limited fashion (maybe an
enumspecifying "full" or "half" width). Or just use the relative size provided on theStatisticItems instead.Should probably hold off merging until after the next build so that rulesets can have a bit longer to prepare for required changes at their end. Would still appreciate review to make sure there's no issues with this direction.