Hides app bars on scroll down and show on scroll up

[akolson]

Summary

This pr implements the hide/show behavior of the top bar and the bottom navigation when a user scrolls down, and show again as they scroll up.

Note: The useScroll from vueUse seems like a more cleaner way to implement the behavior in place of the scroll event handler(as recommended by @MisRob), but it seems it only works for predetermined widths and heights. I am happy to hear any thoughts on possible ways it could still be used. Also happy to hear from @jtamiace @tomiwaoLE about the behavior(hide/show) from a UI/UX perspective.

• Touch device

Screen.Recording.2023-11-21.at.16.59.14.mov

• Non touch device

Screen.Recording.2023-11-21.at.17.08.38.mov

References

Closes #11471

Reviewer guidance

• Run the the apk artifact on this pr.
• Try so scroll up and down in any scrollable screen;
  • on scroll down, the top bar and the bottom navigation are hidden
  • on scroll up, the top bar and the bottom navigation are shown
• This behavior should not be present on any of the web based artifacts(.dmg, .pex, etc) but only on touch devices(apk artifact)

---

Testing checklist

• Contributor has fully tested the PR manually
• If there are any front-end changes, before/after screenshots are included
• Critical user journeys are covered by Gherkin stories
• Critical and brittle code paths are covered by unit tests

PR process

• PR has the correct target branch and milestone
• PR has 'needs review' or 'work-in-progress' label
• If PR is ready for review, a reviewer has been added. (Don't use 'Assignees')
• If this is an important user-facing change, PR or related issue has a 'changelog' label
• If this includes an internal dependency change, a link to the diff is provided

Reviewer checklist

• Automated test coverage is satisfactory
• PR is fully functional
• PR has been tested for accessibility regressions
• External dependency files were updated if necessary (yarn and pip)
• Documentation is updated
• Contributor is in AUTHORS.md

[github-actions]

Build Artifacts

Asset type	Download link
PEX file	kolibri-0.16.0b10.dev0_git.36.gdede7226.pex
Windows Installer (EXE)	kolibri-0.16.0b10.dev0+git.36.gdede7226-unsigned.exe
Debian Package	kolibri_0.16.0b10.dev0+git.36.gdede7226-0ubuntu1_all.deb
Mac Installer (DMG)	kolibri-0.16.0b10.dev0+git.36.gdede7226-0.3.0.dmg
Android Package (APK)	kolibri-0.16.0b10.dev0+git.36.gdede7226-0.1.0-debug.apk
TAR file	kolibri-0.16.0b10.dev0+git.36.gdede7226.tar.gz
WHL file	kolibri-0.16.0b10.dev0+git.36.gdede7226-py2.py3-none-any.whl

[MisRob]

Hi @akolson, thank you, it's looking great overall.

I haven't tested, just reviewed code. I noticed two performance issues that would be good to deal with.

[MisRob]

@akolson

The useScroll from vueUse seems like a more cleaner way to implement the behavior in place of the scroll event handler(as recommended by @MisRob), but it seems it only works for predetermined widths and heights.

" it seems it only works for predetermined widths and heights." What do you mean by that? I think it's okay to use your current implementation, asking only to understand this utility better.

[MisRob]

@akolson As you mentioned, it'd be good to have input from @jtamiace and @tomiwaoLE, particularly on the transition. I'd rather not us adding more customized transition (there are many inconsistent transitions scattered across the whole Kolibri), that's why I created KTransition to encapsulate the transitions we use and are ideally agreed on beforehand with the design team https://main--kolibri-design-system.netlify.app/ktransition/. If this doesn't get addressed in this PR (I think that'd be fine), could you please open a KDS issue where you'd reference your new transition code and made a request for it to be added to KTransition?

[MisRob]

@akolson
Even though, now when I think about the transition more, it depends on whether this is the only of its kind and specific to the app bar only. In that case, it might make more sense to not move it to KDS.

Did you copy it from another place or is it brand new?

[akolson]

@MisRob the transition is specific to the app bars and bottom bars only. For the case of my comment on

The useScroll from vueUse seems like a more cleaner way to implement the behavior in place of the scroll event handler(as recommended by @MisRob), but it seems it only works for predetermined widths and heights.

You need to set the width and height for the target container and also set an overflow. In our context, the width and height of the container is determined at runtime and thus doesn't seem to work. Here is an example

[MisRob]

@akolson

You need to set the width and height for the target container and also set an overflow. In our context, the width and height of the container is determined at runtime and thus doesn't seem to work. Here is an example

Ah I see, that's good to know, thank you. It may be that they need to have width and height set in advance to be able to do calculations.

[akolson]

Yes that was my understanding of the issue

[MisRob]

Thank you @akolson, looking good to me now. I also appreciate some little tweaks you did to naming, it all feels very clear now. As soon as QA confirms, we can merge.

[pcenov]

Hi @akolson when I am using an actual touch screen device if I go to the Library page and scroll quickly all the way down to the bottom of the page the screen starts to flicker:

2023-12-01_17-22-15.mp4

The bottom bar is not always displayed back when I am at Device > Settings (when the 'Set storage limit for auto-download...' is selected):

2023-12-01_19-53-45.mp4

[akolson]

Hi @pcenov for the first video, it doesn't look like the view displayed is that of the app and thus doesn't have the bottom bar.

Were you testing under different set of conditions? If you are using the app view on the browser, you might want to refresh the page whenever you set to that mode as the app view never takes effect immediately(or automatically for that matter). Will also be grateful for clarification on the actual behavior when using the web browser but on a mobile, touchable device as shown in the video. cc @marcellamaki

[akolson]

Looking into issue 2. Thanks

[akolson]

For 2. It turns out that checking the "Set storage limit..." has a behavior that conflicts with the new implementation. See below; clarification also needed here cc @marcellamaki

Set.storoage.limit.scroll.behavior.mov

[marcellamaki]

@akolson - to make sure I'm understanding correctly, is it that on the down scroll, the save changes button is covered a bit, and you have to "scroll extra" (for lack of a better term) so that it appears? If so, I think we should just add a bit of extra height on the bottom of the screen in this scenario. If I'm not understanding, let me know and we can go from there. Thank you!

[akolson]

I am not too sure about the motivation around its implementation but I suspect that it is as you mention. The bigger issue here is that the two implementations(particularly when "set storage limit.." is set) may not exist together with the new(this Pr) since the behavior of one cancels out the behavior of the other thus the bug Peter reported.

[pcenov]

Hi @akolson - sorry, I had uploaded a video for a different PR. I've now uploaded the correct video which is observed on an an Android phone with the Kolibri .apk app installed.

[akolson]

Thanks @pcenov. Let me investigate what could be causing the flicker!

[akolson]

@jtamiace any thought on how we should proceed with the two conflicting behaviors as discovered with #11551 (comment)?

[jtamiace]

Hi @akolson , is it KTransition that is causing the bottom bar to not appear, or the entire 'set storage limit' feature? I'm not sure I understand why these behaviors are conflicting, and also not sure if I have the technical knowledge to find a solution making both behaviors possible at the same time.

I do think the bottom bar needs to be visible on the Device > Settings page even while the 'set storage limit' option is active, since without it, the user would not be able to navigate elsewhere in the app (at least not without using the Android back button or other non-ideal means). I also think it's important for the behavior of the bottom app bar to be consistent with other locations that it appears in the app.

[marcellamaki]

Okay - @akolson and @pcenov, I've dug into this a little bit to try to figure out a way forward, and I think the bar disappearing with the selection of the setting was a pre-existing bug and not a problem with this PR. The KTransition might be adding another layer of complication, but I don't believe it's the root of the problem, as I've replicated in 0.16 (just with a slightly less easey transition). (Edit: Misha has pointed out that Ktransition isn't here yet, so disregard!)

Samson - it seems like the input field is causing something strange with the rendering. One initial suggestion I have that seems to be working after playing around with it a bit, is to adjust the display of the input, so that it has the width it currently has on medium and large screens, but a smaller width on mobile. Not even necessarily with a condition - it seems like just setting some updated width and max-width in the css would be sufficient. This might also be a preferable user experience anyways. I've only been testing this in my dev tools so I am not 100% sure it will work on a physical Android device, but that could be a place to start.

If it does turn out to be this straightforward, I think it would be reasonable to include this in the scope of this PR, Samson. Otherwise, I can open a follow up issue and I feel confident one of our community contributors would love to pick it up :D

[MisRob]

Just a note that KTransition is not used here yet. I was only suggesting we use it instead of the plain transition but that depends on answers to questions I asked on that topic somewhere above. Doesn't seem to be directly related.

[akolson]

Thanks @marcellamaki @jtamiace this makes sense now.

[akolson]

Hi @pcenov!

The bottom bar is not always displayed back when I am at Device > Settings (when the 'Set storage limit for auto-download...' is selected):

has been fixed.

Screen.Recording.2023-12-12.at.22.55.04.mov

Also, I tried to replicate

I am using an actual touch screen device if I go to the Library page and scroll quickly all the way down to the bottom of the page the screen starts to flicker

after the fix for issue 2, but all seems fine now. Please let me know if this isn't the case .

[marcellamaki]

Thanks for the updates here @akolson - the additional code changes seem good to me. As soon as Peter approves on the manual QA side, 👍

[pcenov]

Hi @akolson, I confirm that the issue at Device > Settings is fixed now.
The flicker issue at the Library page is still happening though and I am able to replicate it in all my Android phones plus in Android Studio using a device emulator. To replicate it you just need to have enough content at the Library page and quickly scroll down until you reach the bottom of the page.

[marcellamaki]

@pcenov - have you encountered this on any other builds? I recall several months ago we had an issue where the content cards on the library and topics pages were flickering, and I'm curious if that has come back. It's possible that it is caused by these changes, but also it might be separate, but easily noticed here with the type of testing you are doing. If you are able to see it present in any other testing, letting us know would be helpful for focusing in on the problem. Thank you so much!

[pcenov]

Hi @marcellamaki, no I've not encountered that issue in other builds and it's also not present in the latest Beta 10 Android app so it seems that it's caused by the changes made here.

[akolson]

It looks like the above custom transition animation(from L218 to L241) is causing the glitches on scroll. The obvious work around for this is get rid of the transition animation and keep it simple. @marcellamaki any additional thoughts?

[marcellamaki]

that sounds like a great, simple solution @akolson - let's do that!

[akolson]

HI @pcenov the issue has been fixed as shown in the attached video.

Screen.Recording.2023-12-19.at.09.41.37.mov

[pcenov]

Thanks @akolson - I confirm that this is working correctly now. Good to go!

[pcenov]

Hi @akolson and @marcellamaki - the flickering issue is still happening in Beta 13 - can you confirm that the fix for it has made it into Beta 13? If it has, then it was not fully fixed and I will open a separate issue.

[marcellamaki]

The changes here were included in the beta13 @pcenov - if it has not fully fixed the issue, yes, can you please open a new one that outlines the problems you are seeing, and reference this issue in it so we have the continuity of discussion? thank you!

[akolson]

@pcenov, I tried to replicate the issue on release-0.16.x but It looks like i am unable to based on the initial replication steps you provided. As noted by @marcellamaki, we can do a further analysis based on the issue you open. Thanks