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.

Sign up for GitHub

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

Jump to bottom

Add scrollbar to tabular view #121

Merged
merged 1 commit into from
Feb 16, 2025
Merged

Add scrollbar to tabular view #121

merged 1 commit into from
Feb 16, 2025

Conversation

m12t
Copy link
Contributor

@m12t m12t commented Feb 13, 2025 •
edited
Loading

Closes #78

Changes:

  • Add scrollbar widget and necessary fields to the App struct.
  • Modify app.next_program() and app.previous_program() to
    • Remove the wrapping navigation functionality. I think this is a confusing UX when a scrollbar is visible as it will jump up from bottom to top if a user indexes beyond the last program.
    • Include logic to update the scrollbar position

TODO:

  • Manual testing and automated tests on the scrollbar functionality (all tests pass)
  • Incorporate maintainer feedback

@m12t m12t marked this pull request as ready for review February 14, 2025 04:29
@jfernandez
Copy link
Contributor

@m12t I will take a look soon. In the meantime, could you please squash the commits into a single one?

jfernandez
jfernandez requested changes Feb 15, 2025
View reviewed changes
Copy link
Contributor

@jfernandez jfernandez left a comment

Choose a reason for hiding this comment

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

I tested this, and it's working great. Would it be possible to hide the scrollbar if it's unnecessary? Some users, including myself, will prefer this.

Also, please rework the commit message to explain the change rather than the list of previous commit titles. I want this context to be captured in the commit itself, rather than the Github PR.

Thank you for your work!

@m12t
Add scrollbar widget to the tabular view.
be00bbb 
Rendering of the scrollbar is limited to only when content exceeds the
vertical capacity of the terminal window. This threshold dynamically
adjust if the terminal is resized at runtime.

Wrapping navigation behavior on the table was removed to improve UX.
@m12t
Copy link
Contributor Author

m12t commented Feb 15, 2025

I tested this, and it's working great. Would it be possible to hide the scrollbar if it's unnecessary? Some users, including myself, will prefer this.

Thanks for the feedback, @jfernandez ! That's a good idea. I've updated the code to duck the scrollbar as needed. Resizing the terminal while running doesn't cause any problems since the decision to display or hide the scrollbar is recalculated every iteration of the draw loop and the area.height is taken into account.

Also, please rework the commit message to explain the change rather than the list of previous commit titles. I want this context to be captured in the commit itself, rather than the Github PR.

Done

Thank you for your work!

This was fun, thanks for letting me give it a go! Let me know if you have anything else you'd like help with.

@m12t m12t requested a review from jfernandez February 15, 2025 23:25
@jfernandez jfernandez merged commit 99029e8 into Netflix:main Feb 16, 2025
4 checks passed
@jfernandez
Copy link
Contributor

@m12t looks great! I'm gonna go ahead and cut the next release with this change.

This was fun, thanks for letting me give it a go!

Will do! I'll make enhancement issues for any more ideas I have, or bug fixes around this.

@BrewTestBot BrewTestBot mentioned this pull request Feb 16, 2025
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jfernandez jfernandez jfernandez approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add scrollbar to tabular view
2 participants
@m12t @jfernandez