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

Fluent ScrollBar/ScrollViewer #4119

Merged
merged 13 commits into from
Jun 22, 2020

Conversation

MarchingCube
Copy link
Collaborator

@MarchingCube MarchingCube commented Jun 14, 2020

What does the pull request do?

Ported ScrollBar and ScrollViewer from WinUI.

What is the current behavior?

Well, launch control catalog and see for yourself :)

What is the updated/expected behavior with this PR?

fluent-scroll
image

How was the solution implemented (if it's not obvious)?

Due to sheer amount of states and issues with transitions (can't replace transitions and keep neutral values) I had to implement delays using timers. Hopefully when we fix transitions we could just simplify this a bit.

Also I am not so sure about how to let XAML configure delays for expand/collapse. I don't want to expose yet another property just for it.

I am using a property for expanded state since it is a bit easier to bind it and check for changes (and ScrollViewer needs to know if any ScrollBar is expanded, WinUI has weird setup to workaround it as well so I think our setup is a bit cleaner atm).

Checklist

Breaking changes

Fixed issues

@MarchingCube MarchingCube changed the title WIP: Fluent ScrollBar/ScrollViewer Fluent ScrollBar/ScrollViewer Jun 20, 2020
@MarchingCube MarchingCube marked this pull request as ready for review June 20, 2020 20:53
/// <summary>
/// Defines the <see cref="IsExpandedProperty"/> property.
/// </summary>
public static readonly DirectProperty<ScrollViewer, bool> IsExpandedProperty =
Copy link
Member

Choose a reason for hiding this comment

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

Do we need IsExpanded property?
I am not sure, if it is useful anywhere except template.

Copy link
Member

Choose a reason for hiding this comment

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

I think this will be used when we detect the scrolling is via touch and set this to false... to prevent expansion? @MarchingCube

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Currently it is only used in the template, I've added it mostly to keep API similar between ScrollBar and ScrollViewer. Also might be useful if somebody wants to bind to it. Other option would be a pseudoclass which is basically the same but not public.

Copy link
Member

@danwalmsley danwalmsley left a comment

Choose a reason for hiding this comment

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

LGTM, just a couple of questions.

@@ -193,6 +250,68 @@ protected override void OnApplyTemplate(TemplateAppliedEventArgs e)
}
}

private void InvokeAfterDelay(Action handler, TimeSpan delay)
Copy link
Member

Choose a reason for hiding this comment

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

interesting, i had thought we could use transition delay, etc.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Transitions have several issues right now that make use hard:

  • you can't bind any properties on a transition.
  • in this case we have different delay when collapsing and when expanding. CSS way of solving that is to have two sets of transitions, one that matches non-expanded and one that matches the expanded one. But due to bugs in transition handling replacing transitions cause neutral values to be lost.

@Sorien
Copy link
Contributor

Sorien commented Jun 21, 2020

nice work, one small thing, I did not test it and maybe I'm wrong but from animation at the first post, it looks like that hit zones in collapsed mode are too thin and should be a bit expanded :)

@MarchingCube
Copy link
Collaborator Author

nice work, one small thing, I did not test it and maybe I'm wrong but from animation at the first post, it looks like that hit zones in collapsed mode are too thin and should be a bit expanded :)

Hit zones are the same for both collapsed and expanded modes. So you don't need to be pixel perfect and just having the cursor in the vicinity will expand it.

scrollbar-hitzone

@maxkatz6
Copy link
Member

WinUI also hides scrollbar completely, if scrollviewer is not focused (or its child) and it wasn't expanded before. On scrollviewer pointer over it immediately shows again.

Not sure, if it is important, but I just found it while comparing this one with WinUI.

@maxkatz6
Copy link
Member

Good job by the way, it looks and feels great)

Copy link
Member

@danwalmsley danwalmsley left a comment

Choose a reason for hiding this comment

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

LGTM

@danwalmsley danwalmsley merged commit 847c206 into AvaloniaUI:master Jun 22, 2020
@maxkatz6 maxkatz6 mentioned this pull request Jun 23, 2020
@MarchingCube MarchingCube deleted the fluent-scrollbar branch June 23, 2020 12:03
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.

4 participants