jfmcquade
commented
7 months ago Thanks, @chrismclarke. I like your changes I think, neater to have the collapse handled within header component (in theory the directive approach meant that other elements could have a collapse applied, but I don't really see a use case for that).
To your points:
- Change AppConfig subscription to use more targetted
changesWithInitialValue$variant (easier to tell what's changed)
Nice, I forgot about this
- Calculate header height programatically instead of from defined config (not sure if much benefit or not tbh)
The set heights for the compact and default header variations are hardcoded in the header component now. I'm happy with this, as its the only place they're used, just wanted to clarify that they're not calculated programmatically in the sense of reading from theme variables or elsewhere.
- The code is made a bit uglier through the need for dynamic subscriptions. Do we think the appHeader condense property would actually change at runtime or do we just implement one-time?
Yeh it does seem pretty unlikely we'd want to change this at runtime. But sticking to our model of skins being able to change any app_config property at runtime, it does make sense to keep this dynamic in my opinion, unless there might be an effect on performance.
- I haven't applied additional padding/opacity styling which the original had. Not sure if you want to add back in or not (hadn't previewed locally to inspect)
I'm happy with the look of the new effect, I like the way it looks like it's scrolling off screen.
- Animating based on margin-top is easier to implement, although not sure if its as smooth as animating height... interested to know what you think
It seems to be OK from my testing, with the caveat explained with demos below.
- The pr implementation only tracks scroll events on template pages, and the
ion-contentscroll events is always set ason. It might be worth reconsidering if this can somehow be toggled (possible follow-up), and also worth checking if any other pages might want collapse behaviour (I assume fine not)
Yeh, I did consider both of these points. I think the only way to set scrollEvents on the ion-content dynamically would be to have another subscription to the appConfig in template.page.ts. I was thinking this wasn't worth it, but as the ionic docs suggest that this setting does impact performance, perhaps that is worth implementing? (UPDATE: I have implemented with this commit.)
I agree that it should be fine to only listen for scroll events on templates, at least for now.
Debounce
I've made a minor change to remove the debounce from the scroll events subscription. I can see the motivation for including it, but I think it isn't necessary and causes the UI to feel sluggish: with the debounce there is a noticeable delay before the animation happens, and more complex scroll events (e.g. scrolling repeatedly in multiple directions) don't feel right as they're not broken up into enough sub-events.
If we wanted, we could include the debounce but reduce the value. But from my testing I don't think this is necessary (logging the scroll events shows that the number coming through isn't wildly high).
With debounceTime(50):
Without debounceTime:
Mixed scrolling
One issue that is somewhat mitigated by removing/reducing the debounce time, but not completely, is scrolling quickly with a mixture of up and down. If the header is hidden, for example, and you quickly scroll down and then up, then it does not reappear if the ending height is not higher than the height at the start of the scroll. I think this is acceptable, and is a feature of using scroll events, which are non-infinite in number.
chrismclarke
PR Checklist
Description
Targets #2216 branch (intended to update PR)
Main Changes
changesWithInitialValue$variant (easier to tell what's changed)Additional Considerations
ion-contentscroll events is always set ason. It might be worth reconsidering if this can somehow be toggled (possible follow-up), and also worth checking if any other pages might want collapse behaviour (I assume fine not)Git Issues
Closes #
Screenshots/Videos
If useful, provide screenshot or capture to highlight main changes