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

fix(#299): fix scrolling issue #314

Merged
merged 1 commit into from
Nov 16, 2023

Conversation

wridgeu
Copy link
Contributor

@wridgeu wridgeu commented Aug 5, 2023

Hi there,

I took some time and did some digging again. Found possibly the simplest way to fix #299 for good by letting the renderer take care of clearing the aggregations and the corresponding re-rendering & invalidating of everything. Works quite well.

Longer Story (if interested)

I had another look and to simplify the overview of the involved components/classes I threw together a simple PlantUML (not really UML though :P).

image

What you can see here is that ... in the end the Container responsible for scrolling in each case, is the root sap.m.Page control. Which "could" be fine if it wasn't for how each GrowingEnablement-instance takes care of setting the ScrollDeleagte(the ScrollContainer). This is something I also touched in #299 regarding how they're set in the onAfterRendering and therefore only the last rendered one is set and used which then takes out all of the fun in our scrolling :(.

This could be taken care of (also as already mentioned) by adjusting the entire way the application is built i.e. using root App ... placing the Pages in there ... you know, how any other UI5 SPA is built up. This would result in one ScrollContainer per scrollable /growing sap.m.List thus it'd be fine if each there is only one ScrollContainer per List. Of course, many other ways you could do that, like re-thinking the scroll-UX how many containers there are etc... (not the point though :P)

There are ways in which it could work the way we do it, but that'd involve e.g. changes in the binding (if I've seen that right, observe the reset on the delegate as well as complete control invalidation) and that isn't really our use-case here.

What would also work is manually taking care of actively triggering the corresponding onAfterRendering method or completely re-rending the control (which in turn does all the same ... at last). This could be done in the onPatternMatched-event handler like so:

// manual "onAfterRendering"
this.byId("listHotPackages")?._oGrowingDelegate.onAfterRendering() // or
// manual control invalidation
this.byId("listHotPackages")?.invalidate() // "cleaner" than the line above at least 

This rather manual approach works, but feels a bit more clunky when it comes to the already rendered clones (list items). For example: when you're in the "All Packages"-tab and scroll all the way down (render all the items) and then go back & forth (so you end up on "All Packages" again, you'd see that all previously, additionally rendered items are thrown away and you can do all the scrolling again.

The adjustment with the Renderer and letting it take care of it, worked way better here (despite the docs explicitly saying not to use it in this scenario "clearControlAggregation: [...] For a sap.m.NavContainer it should be false.").

demo

PlantUML @startuml skinparam componentStyle rectangle

[Hot Packages] as HP
[All Packages] as AP

[GrowingDelegate Hot] as GH
[GrowingDelegate All] as GA

[ScrollDeleagte Hot] as SH
[ScrollDeleagte All] as SA

[Scroll Container] as SC

HP <-- GH
GH <-- SH
SH <-- SC

AP <-- GA
GA <-- SA
SA <-- SC

GH <..> GA : not equal (good)
SA <..> SH : equal (bad)

note right of [GH]
GrowinDelegate is from GrowingEnablement
(The delegate that sits on the List control in ._oGrowingDelegate)
end note

note right of [SH]
ScrollDeleagte is from ScrollEnablement
(The scroll delegate which sits on the GrowingDeleagte in _oScrollDelegate)
end note

note right of [SC]
Root sap.m.Page control of App
end note

note right of [HP]
The corresponding sap.m.List control
(same for All Packages)
end note
@enduml

@wridgeu wridgeu requested a review from marianfoo August 5, 2023 16:16
@wridgeu wridgeu self-assigned this Aug 5, 2023
@marianfoo marianfoo merged commit bdf28fe into ui5-community:main Nov 16, 2023
2 of 3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

growingScrollToLoad is not triggered on "Hot" and "All Packages" View
2 participants