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.

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

ListViewBase Header Behaviors Port #136

Merged
merged 10 commits into from
Jul 17, 2023
Merged

Conversation

michael-hawker
Copy link
Member

@michael-hawker michael-hawker commented Jul 11, 2023

Requires #121

Fixes #112

Needed for CommunityToolkit/Labs-Windows#418

Brings over the StickyHeaderBehavior, QuickReturnHeaderBehavior, and FadeHeaderBehavior from the Toolkit missing in the original port. Adds a sample for each built upon ListView, GridView, and HeaderedTreeView.

Having issues building UWP head for Behaviors component though... WASDK worked fine, no obvious error message in build output, will see if CI highlights anything different...

  • May investigate a quick base class that shares the logic between StickyHeaderBehavior and QuickReturnHeaderBehavior...
  • May investigate if it's better to attach to what's in the Header instead so it can work with basically anything in a ScrollViewer?

@michael-hawker
Copy link
Member Author

Opened #137 for tracking the UWP head issue, not introduced in this PR or the other one, so we'll investigate later.

Didn't have a chance today to finish looking into clean-up for this. Will take a quick look in the morning and test on WASDK head. Once this is merged, I can finish the DataTable PR in Labs and will see if I have issues referencing UWP Behaviors package there in samples...

@michael-hawker
Copy link
Member Author

Clean-up is looking good here, have a base class for all three Header behaviors now that does the heavy lifting of finding the required elements.

Then going to see if I can flip the paradigm a bit to simplify the code a tiny bit and also enable it for other scenarios with just ScrollViewer instead of needing a ListViewBase...

@michael-hawker
Copy link
Member Author

Making the element to attach to be the Header over the ListView was easy enough. Adding a new sample to test with HeaderedItemsControl which wouldn't have been possible before, so that's cool. 🙂

Last test will be to see if it'll work with anything that's within a ScrollViewer (ideally at the top)...

Could also mean we could attempt to try with the Footer as well... though that may need we an inversion of direction bool or something to control that... That may be a future thing to investigate.

@michael-hawker
Copy link
Member Author

Footer scenarios need all new math, so maybe it's just easier to have different classes (with different names) for those that work from the bottom up.

Could change the base class though from 'HeaderBehaviorBase' to 'ScrollViewerCompositionBehaviorBase' though (albeit it's wordier, it's more accurate)...

Everything is working except the QuickReturn in the new scenario on first load (if I change tabs and comes back it works fine), building all the samples so I can test if it's a WASDK quirk vs. UWP (need all due to #137)

@michael-hawker
Copy link
Member Author

michael-hawker commented Jul 13, 2023

Ugh, I can't test if it's a UWP issue with QuickReturn in ScrollViewer due to CommunityToolkit/Tooling-Windows-Submodule#100

Edit: Maybe I can swap it with a rectangle for now to test quick locally...

@michael-hawker
Copy link
Member Author

K, was able to test locally in UWP with a rectangle, doesn't work. So, it's something specific to the initial load vs. subsequent usage... will need to investigate, but maybe will just file an issue since it's a new scenario. It's cool when it works though!

@michael-hawker michael-hawker marked this pull request as ready for review July 13, 2023 00:24
@michael-hawker michael-hawker added the enhancement New feature or request label Jul 13, 2023
Add doc and Samples showing for ListView, GridView, and HeaderedTreeView
Tested on WASDK (uses composition, so won't work on Uno)
Having issues with UWP head?
…ew abstract HeaderBehaviorBase helper class

Is responsible for finding needed controls and properties for setting up composition animation. 90% of these two classes were doing the same thing, so now only the logic related to setting up/manipulating the composition animation is in each subclass.
Going to investigate if FadeHeaderBehavior is similar enough to reuse as well.
Was much more aligned then it looked at first glance. Think it was first, so some things were just done locally in scope vs. cached from the other classes. At least now we can clean things up all together easily if we want to optimize things.

Main difference is some of the event hooks, so if they're not needed we may want flags/options for those in the base class to enable.
…new scenarios enabled by changes

Show a HeaderedItemsControl with a StickyHeader
Show a QuickReturn of an element that's at the top of a ScrollViewer (doesn't load properly on first load though??)
@michael-hawker michael-hawker force-pushed the llama/header_behaviors branch from ae2ea78 to 6033436 Compare July 13, 2023 16:46
@michael-hawker
Copy link
Member Author

Weird... same issue that happened on #140 (comment) but on the WinUI 2 pipeline vs. 3...

@michael-hawker
Copy link
Member Author

This may have been effected by CommunityToolkit/Tooling-Windows-Submodule#96 - probably didn't notice as I was testing the samples maybe? Can't remember. Either that or it's a regression?

@niels9001
Copy link
Collaborator

Minor design tweaks to the samples: #142

@michael-hawker While testing this, some observations:

  • The autofocus behavior wasn't working for me? (I guess the TextBox is supposed to be highlighted?)
  • Most of these samples do not work/load on WASM (but I guess that's expected?)
  • UWP didn't want to compile, with vague errors (might be my machine though, switching branches sometimes confuses VS):
    image

image

@michael-hawker
Copy link
Member Author

@niels9001 see #137 there's some weird issue with the Behaviors UWP head, building the entire sample app works fine.

I didn't touch anything with the auto focus behaviors. They don't work in the existing Toolkit app either, so I'm thinking maybe the OS broke these or something? @vgromfeld is this anything you're aware of?

The Header behaviors won't work on WASM (or Uno) as they all use composition, so that's expected. Maybe should call that out in the docs, eh?

* Design tweaks

* Removing background theme brushes
@vgromfeld
Copy link
Contributor

I didn't touch anything with the auto focus behaviors. They don't work in the existing Toolkit app either, so I'm thinking maybe the OS broke these or something? @vgromfeld is this anything you're aware of?

We're not aware of anything on our side. I realized that we're still using our initial implementation from before our contribution to the toolkit (😓) but the two implementations are the same. We do not have any issues on our side within an UWP project.

@michael-hawker
Copy link
Member Author

@niels9001 did I address your questions? We good for this? (Want to add it to at least an existing sample on the DataTable before trying to merge that.)

@michael-hawker michael-hawker merged commit 338be80 into main Jul 17, 2023
@delete-merged-branch delete-merged-branch bot deleted the llama/header_behaviors branch July 17, 2023 18:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Port ListView Header Behaviors
3 participants