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

Scrolling TabBar #5022

Open
wants to merge 5 commits into
base: develop
Choose a base branch
from
Open

Conversation

robob27
Copy link
Contributor

@robob27 robob27 commented Nov 10, 2024

  • Modify TabBar to allow tabs to be scrolled instead of wrapping by setting wrap=false in the TabBar attributes. wrap defaults to true so any existing instances of TabBar should be unaffected (tested with launcher and control-panel and they're looking good).
  • Add tests for Tab/TabBar

I made a little demo script with a scrolling TabBar alongside a wrapping one with a resizable window so the behavior can be easily tested (requires this branch). Testers should try changing autoarrange_subviews in the script and ensure it doesn't break things, play around with the options, and double check that existing consumers of this widget like launcher and control-panel doesn't get messed up in some way that I missed.

Some notes on the expected behavior:

  • If the window is resized while the tabs are scrolled to the right, it will just reset the scroll offset back to 0
  • The scroll left/scroll right buttons should only be shown when needed. If resizing the window, the scroll right button should disappear as soon as the last tab is fully in view
  • When switching tabs with hotkeys or by clicking, the selected tab should be scrolled into view + a slight (customizable) offset to show part of the next tab. The default offset seems generally appropriate but didn't work well with a TabBar with a small width, so I made it possible to customize it.

@robob27
Copy link
Contributor Author

robob27 commented Nov 10, 2024

pre-commit.ci autofix

@robob27
Copy link
Contributor Author

robob27 commented Nov 10, 2024

I should probably make it possible to customize the colors of the scroll labels as well 🤔 Done.

Comment on lines +6236 to +6239
:scroll_left_text: The text to display on the left scroll label.
Defaults to "<<<".
:scroll_right_text: The text to display on the right scroll label.
Defaults to ">>>".
Copy link
Member

Choose a reason for hiding this comment

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

As a matter of style consistency, I don't think this should be configurable. I think players would expect this to be constant in all situations where it appears.

I also suggest making the horizontal scroll icon look more like this:
image

perhaps with only one left/right triangle if two triangles stacked vertically doesn't fit well. The code for that example screen is here:

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah yeah that's a much nicer symbol. Will fix!

Copy link
Member

@myk002 myk002 left a comment

Choose a reason for hiding this comment

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

This looks awesome! Thank you!

Scrolling is very important to get right. Could you add some unit tests in test/library/gui/widgets.TabBar.lua (need to create a new file, but can be based on test/library/gui/widgets.Scrollbar.lua)

@robob27 robob27 marked this pull request as draft November 17, 2024 00:05
@robob27 robob27 marked this pull request as ready for review November 17, 2024 00:26
@robob27
Copy link
Contributor Author

robob27 commented Nov 17, 2024

@myk002 Added a bunch of tests for the new scroll behavior as well as some tests for existing behavior. I think I covered pretty much everything? But let me know if you see something I missed and I'm happy to add it 😄

@robob27 robob27 requested a review from myk002 November 17, 2024 00:28
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.

2 participants