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: [CommandBar] supports setting directions if needed. #872

Merged
merged 11 commits into from Jul 12, 2023

Conversation

CorvusYe
Copy link
Contributor

@CorvusYe CorvusYe commented Jul 1, 2023

Add direction to CommandBar to support verticality.
Sometimes, we need to place the command bar on the side.
Just like this:
image

When we set direction to Axis.vertical, the isCompact will default to true, if it is not set.

Pre-launch Checklist

  • I have updated CHANGELOG.md with my changes
  • I have run "dart format ." on the project
  • I have added/updated relevant documentation

@CorvusYe
Copy link
Contributor Author

CorvusYe commented Jul 2, 2023

Sorry, this version can't cover all options, uch as overflowBehavior: CommandBarOverflowBehavior.dynamicOverflow.

If you agree with this feature, I will make another PR when all options are compatible.

Sorry for the previous mistake again.

@CorvusYe CorvusYe closed this Jul 2, 2023
@bdlukaa
Copy link
Owner

bdlukaa commented Jul 2, 2023

I like the concept and would love to see this working :)

@CorvusYe
Copy link
Contributor Author

CorvusYe commented Jul 3, 2023

@bdlukaa
Now it is compatible with scrolling and dynamicOverflow.
But, I have a question about whether to expand the scrollDirection attribute in HorizontalScrollView
This makes me hesitate.

@CorvusYe CorvusYe reopened this Jul 3, 2023
@bdlukaa
Copy link
Owner

bdlukaa commented Jul 5, 2023

HorizontalScrollView is just a helper widget that makes scrolling horizontally smoother. I don't see a problem using only SingleChildScrollView on vertical command bars.

I'm not sure how resolving the mouse scroll delta for a better scrolling effect works for vertical scroll views tho. iirc, Flutter is working on 2D scrolling, which should affect how horizontal scrolling behaves and we, sometime soon, may get rid of HorizontalScrollView at all.

@CorvusYe
Copy link
Contributor Author

CorvusYe commented Jul 5, 2023

Flutter is working on 2D scrolling, which should affect how horizontal scrolling behaves and we, sometime soon, may get rid of HorizontalScrollView at all.

Does this mean that HorizontalScrollView may be deprecated in the future?
The performance experience of SingleChildScrollView in the vertical direction is still inferior to that in the horizontal direction.

Here are some effects of using vertical scrolling.

  • When I use the mouse scroll wheel in normal mode, there is a slight jumping sensation.
20230705_113435.mp4
  • But when I hold down the shift key, the performance is as smooth as when it's horizontal.
20230705_113510.mp4

@CorvusYe
Copy link
Contributor Author

CorvusYe commented Jul 5, 2023

What I don't quite understand is why this event only triggers when it reaches the top and bottom.

Do I need to make any further changes?

@bdlukaa
Copy link
Owner

bdlukaa commented Jul 5, 2023

I tried your implementation with every example in https://bdlukaa.github.io/fluent_ui/#/surfaces/command_bar, but most of them throw a render box issue :/

@CorvusYe
Copy link
Contributor Author

CorvusYe commented Jul 5, 2023

In my case, the exterior has height constraints. Is it possible that this is the reason?Or am I missing any key elements that are not compatible with 😂

@CorvusYe
Copy link
Contributor Author

CorvusYe commented Jul 6, 2023

It seems that height constraints are still necessary I wrote examples of vertical according to the horizontal usage and following behind each one.
image

Do you have any thoughts on the hiding of vertical scrollbars? It looks very oppressive now.

@bdlukaa
Copy link
Owner

bdlukaa commented Jul 6, 2023

You can wrap the scroll view in a

ScrollConfiguration(
 behavior: ScrollConfiguration.of(context).copyWith(scrollbars: false),
 child: ...,
),

@CorvusYe
Copy link
Contributor Author

CorvusYe commented Jul 6, 2023

@bdlukaa
Thanks for all the support you've given me.
Not only for this PR, but also for my personal project which dependents fluent_ui.

@bdlukaa bdlukaa merged commit f3381b3 into bdlukaa:master Jul 12, 2023
3 checks passed
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.

None yet

2 participants