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

feat: Basic Scrolling in Tab Menu #3

Closed
wants to merge 14 commits into from
Closed

Conversation

bennjii
Copy link
Contributor

@bennjii bennjii commented May 23, 2023

I don't usually do PR's, but I thought I'd have a crack at this one.
This PR Addresses #2 in basic terms, as follows:

This PR contains:

  1. Scrolling within tab view using scroll wheel
  2. Icons for moving back and forth for those without scroll wheel (Implemented in menubar)

Note: Added top_bar_ltl_ui for the < icon incase left alignment is prefered.

  1. .click() for tab elements will scroll them into view.

Implementation:

Using a MPSC (std::sync::mpsc::channel::<f32>()) to share data between the menu elements > and <, and the ScrollArea. The tab menu is now wrapped in a ScrollAreawith an always hidden scroll bar.

Issues with Implementation:

I am not experienced enough with egui to know how to find the fitted width of an element's contents, and so scrolling rightward will permit over-scroll in some cases as the ui.available_width() is greater than the consumed width.

Furthermore, as the scroll area's offset is stored using ctx().memory_mut(), I am unsure of how to update this value to match the scroll when using a mouse - and so, after using the mouse to scroll, and again using the chevrons, the scroll will jump to its previous state. Any help on this is much appreciated.

@emilk
Copy link
Member

emilk commented May 30, 2023

Hi, thanks for the PR!

This seems like a really nice start.

Some thoughts:

We should hide the scroll-buttons unless they are needed. It can be figured out from https://docs.rs/egui/latest/egui/containers/scroll_area/struct.ScrollAreaOutput.html but the results need to be stored until the next frame.

I think the left-arrow should be moved to the left of the tabs. This is how Firefox does it, for instance.

Configure your editor to run cargo clippy for you. It really helps!

@bennjii
Copy link
Contributor Author

bennjii commented Jun 1, 2023

Yep I've made these changes, I want to move the right button to the right side but for some reason using painter in the UI context doesn't show anything...? Preferably, we make both buttons positioned absolute to prevent jarring UI changes when they're shown/hidden and allows for a gradient 'overlay' like in the Firefox example.

What do you suggest?

examples/advanced.rs Outdated Show resolved Hide resolved
examples/advanced.rs Outdated Show resolved Hide resolved
src/container/tabs.rs Outdated Show resolved Hide resolved
src/container/tabs.rs Outdated Show resolved Hide resolved
src/container/tabs.rs Outdated Show resolved Hide resolved
src/behavior.rs Outdated Show resolved Hide resolved
src/container/tabs.rs Outdated Show resolved Hide resolved
@bennjii
Copy link
Contributor Author

bennjii commented Jun 2, 2023

Okay, I've implemented most of the changes (work in progress) - the thing I'm struggling with is to have the central 'scrollable div' fill the space, whist also allowing the outside button menu's to fill their 'somewhat constant' space.

In CSS I'd implement a flex for each with a flex-shrink: none on the outer, etc. Though here we almost need to pre-render both outside components to determine their size (as it can change depending on context such as scroll offset). Following which, allocating fixed space to each component. I don't know if it is possible to create a 0-width inner and post-paint into it after the menu sides are evaluated or something similar, but it feels like an inflexible solution.

I've attempted to fix the width of the scroll menu and add a 25pt padding to either side (which is bad as it doesnt consider flexible menu sizes) but the scroll menu will leave the 25pt padding and continue to push the leftmost arrow outside the div regardless, or overlay it making it hidden.

Optimally, the scroll bar would take up the entire tab space - with both menu icons being an overlay upon it, and the scroll menu having an internal padding equivalent to the size of each menu side, this would allow for a gradient background to fade in the icons as well, but I don't know how to prioritise layering here.

Edit: I notice Z-Layer ordering is being worked on here; emilk/egui#2633

@bennjii
Copy link
Contributor Author

bennjii commented Jun 2, 2023

Okay, I've got a working implementation - feel free to change the code, but essentially I've wrapped the scroll in a viewport with a custom width specification. Each side item has an assumed size of 20.0f, although this is set as a constant so can be changed beforehand based on a pre-render or whatnot.

The reveal works through manually overriding the offsets to reduce any jarring sliding by the frame size changes. Notably, through frame-by-frame edits of the consume size, we can perform a reveal for the icon which looks quite nice but looses its effect when scrolling small amounts due to its partial revelation...

The same reveal implementation for the left could similarly be introduced to the rightmost element to hide it when reaching the endpoint.

@bennjii bennjii requested a review from emilk June 3, 2023 07:40
@emilk
Copy link
Member

emilk commented Jul 4, 2023

I tried making some commits to this branch in order to get it into a mergable state, but I can't because you made the PR from your own main 😬

@bennjii
Copy link
Contributor Author

bennjii commented Jul 4, 2023

Ah, should I make a new branch for you?

@emilk
Copy link
Member

emilk commented Jul 5, 2023

If you want help resolving the remaining merge issues :)

@bennjii bennjii mentioned this pull request Jul 5, 2023
@bennjii
Copy link
Contributor Author

bennjii commented Jul 5, 2023

I've created one called scroll-indicators on my repo, #9

@emilk
Copy link
Member

emilk commented Jul 6, 2023

Thanks! I've pushed a merge fix to it

Closed in favor of #9

@emilk emilk closed this Jul 6, 2023
emilk added a commit that referenced this pull request Nov 10, 2023
* Closes #3

---------

Co-authored-by: Emil Ernerfeldt <[email protected]>
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