Scrollbar and ScrollArea #1614
Scrollbar and ScrollArea #1614
Conversation
MichaelMauderer
added
Difficulty: Intermediate
|
I tried the scroll bars and there are some things that are non-standard and could be improved in my opinion:
|
|
I ran our test scenarios on this and everything works, except that I could not edit the project name. The same issue was observed on an unrelated branch. It is likely already present on develop. |
| // Mouse IO - Dragging | ||
| drag_movement <- mouse.translation.gate(&base_frp.is_dragging_any); | ||
| drag_delta <- drag_movement.map2(&base_frp.track_max_width, |delta,width| | ||
| (delta.x + delta.y) / width | ||
| ); | ||
| drag_delta <- drag_delta.gate(&base_frp.is_dragging_track); | ||
| drag_animation <- drag_delta.constant(false); | ||
|
|
||
| // Mouse IO - Event Evaluation | ||
| should_animate <- any(&click_animation,&drag_animation); | ||
|
|
||
| drag_center_delta <- any(&drag_delta,&click_delta); | ||
| drag_update <- drag_center_delta.map2(&normalised_track_bounds,|delta,Bounds{start,end}| | ||
| Bounds::new(start+delta,end+delta) | ||
| ); | ||
|
|
||
| is_in_bounds <- drag_update.map(|value| should_clamp_with_overflow(value,&None)); | ||
|
|
||
| new_value_absolute <- all(&frp.set_overall_bounds,&drag_update).map(|(bounds,Bounds{start,end})| | ||
| Bounds::new( | ||
| absolute_value(&(*bounds,*start)),absolute_value(&(*bounds,*end))).sorted() | ||
| ).gate(&is_in_bounds); | ||
|
|
||
| new_value_lower <- new_value_absolute.map(|b| b.start); | ||
| new_value_upper <- new_value_absolute.map(|b| b.end); | ||
|
|
||
| track_position_lower.target <+ new_value_lower.gate(&is_in_bounds); | ||
| track_position_upper.target <+ new_value_upper.gate(&is_in_bounds); | ||
|
|
||
| track_position_lower.skip <+ should_animate.on_false(); | ||
| track_position_upper.skip <+ should_animate.on_false(); | ||
|
|
There was a problem hiding this comment.
This looks like pretty complex FRP - it seems it includes dragging etc. Why such things as dragging are not completely handled by slider instead? I was thinking that this component would just wrap slider and just set a few FRP values to it, without its own complex FRP utility. Also, there are no docs describing what really these functions do, which makes it much harder to understand - it contains some dragging, some mouse IO, while all of these things should be handled by the slider component.
There was a problem hiding this comment.
The actual drag implementation and underlying logic is shared. But this is the logic how to interpret it, which is different in each component: (1) single number selectors have clicking at the target and dragging only on the background (2) range slider need to act on dragging either the track piece or the edges of the track piece (3) the scrollbar needs to have dragging of the track piece and "jumping" when clicking on some other part of the background.
That is essentially what is encoded here. Maybe some things could be refactored and abstracted, but not too much.
There was a problem hiding this comment.
Why cant these things be just coded in (moved to) the slider implementation so you'd be able to choose which mode you wants with FRP settings? Just like choosing colors, we could tell "I want this slider to jump to the place when clicking the background". This should be part of the sldier configuration rather than extracted here imo. Can we move it there, please?
|
Also, from the visual point of view, our sliders should not have background, should not have shadows and should have rounded endings:
Also, the points raised by Felix are good too! |
|
Also, on the Felix's gif, there is a visual artifact visible (this is a max scroll):
|
… scrolling, optimise click jump length.
|
I made some heavy design changes to the scrollbars and added scroll areas to the PR because they are closely related: |
s9ferech
force-pushed
the
wip/mm/ide-1612-implement-scrollbars
branch
from
b14819e to
9aef6c9
Compare
It would likely take me long to fix this test because it now depends on the existence of a `Scene`. The test also not very important. Therefore, I decided to delete it.
|
Here is a recording that shows the interactions in a slightly better quality. |
|
@s9ferech Looks absolutely stunning <3 |
Original commit: enso-org/ide@9bf3843
Pull Request Description
Add a re-usable scrollbar component for internal use. Can be tested in the
sliderdemo scene athttp://localhost:8080/?entry=slider[ci no changelog needed]
Peek.2021-06-09.14-57.mp4
Checklist
Please include the following checklist in your PR:
CHANGELOG.mdwas updated with the changes introduced in this PR.