Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[GridSplitter] Fix #339: resize both items when they are fixed size to avoid the change overflowing to the irrelevant Star-sized item #342

Merged
merged 9 commits into from
Mar 28, 2024
Merged

[GridSplitter] Fix #339: resize both items when they are fixed size to avoid the change overflowing to the irrelevant Star-sized item #342

merged 9 commits into from
Mar 28, 2024

Conversation

abdes
Copy link
Contributor

@abdes abdes commented Feb 23, 2024

Fixes

Fixes #339

Please see #339 for a detailed description of the problem with video of the interaction.

  • Formatted the GridSplitter experiment XAML code with XAMLStyler to avoid unnecessary diffs in the history. Please note that this is theoretically unnecessary, but it should be done anyway given that the project requires XAMLStyler style to be enforced.
  • Augmented the GridSplitter sample to add one more column and set the last 2 columns to fixed size to demonstrate the issue before the fix and the resolution after the fix.
  • Fixed the issue by making the GridSplitter resize both current and sibling when they are both fixed size.
  • Verified that the new behavior honors the minimum size constraint.

PR Type

What kind of change does this PR introduce?

Bugfix
Code style update (formatting)

Sample app changes

What is the current behavior?

Please see #339 for a detailed description of the problem with video of the interaction.

What is the new behavior?

GridSplitter.339.fixed.mp4

PR Checklist

Please check if your PR fulfills the following requirements:

  • Created a feature/dev branch in your fork (vs. submitting directly from a commit on main)
  • Based off latest main branch of toolkit
  • Tested code with current supported SDKs
  • New component
    • Documentation has been added
    • Sample in sample app has been added
    • Analyzers are passing for documentation and samples
    • Icon has been created (if new sample) following the Thumbnail Style Guide and templates
  • Tests for the changes have been added (if applicable)
  • Header has been added to all new source files
  • Contains NO breaking changes

Other information

@abdes
style: auto-format only with XAML styler
4460128
@abdes
Augment the sample with 2 fixed-size columns and a splitter
dc4d6ab
@abdes
fix: resize both current and sibling when they are both fixed size
8dd6176 
Fixes CommunityToolkit#339
If the vector contains another item with Star sizing, it's not enough
to just change current. The change will flow to the Star sized item
and not to the sibling if the sibling is fixed-size. We need to explicitly
apply the change to the sibling.
@abdes
Copy link
Contributor Author

abdes commented Feb 23, 2024

@dotnet-policy-service agree

@abdes abdes changed the title Fix Grid Splitter #339: resize both items when they are fixed size to avoid the change overflowing to the irrelevant Star-sized item [GridSplitter] Fix #339: resize both items when they are fixed size to avoid the change overflowing to the irrelevant Star-sized item Feb 23, 2024
@michael-hawker
Copy link
Member

@abdes it appears you applied different XAML Styler settings than the for the file we have in the repo, ensure your plugin is configured to look upwards in the folder structure for settings files.

I also don't think this behavior is correct. If the limit has been reached for the size a column should grow, no more movement should occur. Here we see the 1 & 2 columns being affected by the changes to the 3 & 4 now, i.e. the third column growing more as you continue to drag to the right and pushing the other two out of the way.

Once the 4th column is the smallest, it should no longer update anything else.

There is ambiguity in if the last column should be able to go beyond the size of the grid. I believe in WPF, it would extend the size of the grid itself to grow beyond the original bounds.

@abdes
style: re-apply XAML style using script
b36053f
@abdes
fix: check constraints before allowing the resize to proceed
c8c7cae 
Fixes CommunityToolkit#339
If the vector contains another item with Star sizing, it's not enough
to just change current. The change will flow to the Star sized item
and not to the sibling if the sibling is fixed-size. We need to explicitly
apply the change to the sibling.
Now also checks the constraints before allowing the resize.
@abdes
Copy link
Contributor Author

abdes commented Feb 24, 2024 •
edited
Loading

Got it.

  • Reverted the changes on styling. Just used the PowerShell script. Could not find how to make the VS Extension auto-find the XAMLStyler config.

  • For the resizing, now it checks the constraints (for both the current and the sibling) before allowing the resize to proceed. Previously, the check was only done for the row/column being changed. In fact it should be done for both because if any of the two impacted rows/columns cannot be resized then the other one should not be resized as well.

GridSplitter.339.fixed.mp4

@michael-hawker please check the new implementation

@abdes
Copy link
Contributor Author

abdes commented Mar 18, 2024

Can we get a reviewer to move this forward please?
I've attached videos, met all requirements and am ready to make any additional modifications needed.

@michael-hawker michael-hawker added this to the 8.1 milestone Mar 19, 2024
@michael-hawker michael-hawker self-requested a review March 19, 2024 18:08
@Arlodotexe Arlodotexe requested review from Arlodotexe and removed request for michael-hawker March 26, 2024 20:52
@abdes
Copy link
Contributor Author

abdes commented Mar 27, 2024

@Arlodotexe
Please approve the workflow...

Arlodotexe
Arlodotexe approved these changes Mar 28, 2024
View reviewed changes
Copy link
Member

@Arlodotexe Arlodotexe left a comment

Choose a reason for hiding this comment

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

Tested locally under Uwp, Wasdk and Wasm. Code looks good and it does fix the issue. Let's get this merged! 🎉

@Arlodotexe Arlodotexe merged commit 216ba41 into CommunityToolkit:main Mar 28, 2024
9 checks passed
@abdes abdes deleted the fix/grid-splitter-339 branch March 28, 2024 05:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Arlodotexe Arlodotexe Arlodotexe approved these changes

Assignees

@Arlodotexe Arlodotexe

@michael-hawker michael-hawker

Labels
None yet
Projects
Toolkit 8.x
Status: ✅ Done
Milestone
8.1
Development

Successfully merging this pull request may close these issues.

GridSplitter with '1*'-<sized>-<sized> columns does not resize as expected
3 participants
@abdes @michael-hawker @Arlodotexe