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

improvement(browse): re-implement scrolling #17839

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

david-allison
Copy link
Member

@david-allison david-allison commented Jan 17, 2025

Purpose / Description

Cause: #16620 (da72986) changed from a ListView to a RecyclerView and scrollbars were not included

NOTE: commit above message is incorrect due to a bad fixup

Fixes

Approach

recycler-fast-scroll [Apache 2.0] was unmaintained but seemed like a reasonable solution.

So it was vendored, refactored and used

https://github.com/pluscubed/recycler-fast-scroll/tree/master

How Has This Been Tested?

S21. Colors are the same in night and light mode. Performance appears to be GREAT, but we can probably improve further.

Learning (optional, can help others)

I could not get fastScrollEnabled to work on a RecyclerView, and the visual indicator without allowing a user to scroll quickly was insufficient

I tried com.simplecityapps.recyclerview_fastscroll.views

  • Rendering was slow
  • But it extended RecyclerView, rather than needing another component

Checklist

  • You have a descriptive commit message with a short title (first line, max 50 chars).
  • You have commented your code, particularly in hard-to-understand areas
  • You have performed a self-review of your own code
  • UI changes: include screenshots of all affected screens (in particular showing any new or changed strings)
  • UI Changes: You have tested your change using the Google Accessibility Scanner

Added in f0e4ceb or flags
This was reverted, but the padding remained
Cause: 16620 (da72986) changed from a ListView to a RecyclerView and scrollbars
were not included

NOTE: commit message is incorrect due to a bad fixup

====
Fix

I could not get  `fastScrollEnabled` to work, and only
a visual indicator was insufficient

`recycler-fast-scroll` [Apache 2.0] was unmaintained
but seemed like a reasonable solution.
So it was vendored, refactored and used

https://github.com/pluscubed/recycler-fast-scroll/tree/master

Fixes 17786

=====

I tried `com.simplecityapps.recyclerview_fastscroll.views`
* Rendering was slow
* But it extended RecyclerView, rather than needing another
 View
@david-allison david-allison added the Review High Priority Request for high priority review label Jan 17, 2025
@david-allison david-allison marked this pull request as ready for review January 17, 2025 11:16
* See the License for the specific language governing permissions and
* limitations under the License.
*
* https://github.com/pluscubed/recycler-fast-scroll/blob/3de76812553a77bfd25d3aea0a0af4d96516c3e3/library/src/main/java/com/pluscubed/recyclerfastscroll/RecyclerFastScroller.java
Copy link
Member Author

Choose a reason for hiding this comment

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

Note: this is vendored code, unsure how you want to review it

@david-allison david-allison added Needs Author Reply Waiting for a reply from the original author and removed Review High Priority Request for high priority review labels Jan 17, 2025
@david-allison
Copy link
Member Author

I'll want to dive into the implementation. It didn't like my 60k card deck

@david-allison david-allison marked this pull request as draft January 17, 2025 20:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Needs Author Reply Waiting for a reply from the original author
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Browser regression: scrollbar missing in alpha 5
1 participant