Skip to content

Remove animations from RangeSelector #3611

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 6 commits into from
Dec 11, 2020
Merged

Remove animations from RangeSelector #3611

merged 6 commits into from
Dec 11, 2020

Conversation

Rosuavio
Copy link
Contributor

@Rosuavio Rosuavio commented Dec 8, 2020 •
edited by michael-hawker
Loading

Removes RangeSelector's usage of the Animations project.

PR Type

What kind of change does this PR introduce?

  • Other... Please describe: Remove animation from RangeSelector tooltip, reduce deps for RangeSelector

What is the current behavior?

The RangeSelector uses the animations project for a short fad-in fade-out animation for the tool tip.

What is the new behavior?

The RangeSelector does not use the animations project, and there is no fading for the tooltip.

PR Checklist

Please check if your PR fulfills the following requirements:

  • Tested code with current supported SDKs
  • Pull Request has been submitted to the documentation repository instructions. Link:
  • Sample in sample app has been added / updated (for bug fixes / features)
  • Tests for the changes have been added (for bug fixes / features) (if applicable)
  • Header has been added to all new source files (run build/UpdateHeaders.bat)
  • Contains NO breaking changes

Other information

No one controls seems to use animations like this.

@ghost
Copy link

ghost commented Dec 8, 2020

Thanks RosarioPulella for opening a Pull Request! The reviewers will test the PR and highlight if there is any conflict or changes required. If the PR is approved we will proceed to merge the pull request 🙌

@ghost ghost requested review from michael-hawker, azchohfi and Kyaa-dost December 8, 2020 20:34
@Rosuavio Rosuavio mentioned this pull request Dec 8, 2020
Closed
@Rosuavio Rosuavio linked an issue Dec 8, 2020 that may be closed by this pull request
Remove Animations Package reference from Controls package #3578
Closed
@Rosuavio Rosuavio mentioned this pull request Dec 8, 2020
Closed
@michael-hawker michael-hawker added this to the 7.0 milestone Dec 8, 2020
@ghost ghost added the in progress 🚧 label Dec 8, 2020
@Rosuavio Rosuavio removed a link to an issue Dec 8, 2020
Remove Animations Package reference from Controls package #3578
Closed
@michael-hawker michael-hawker linked an issue Dec 8, 2020 that may be closed by this pull request
Remove Animations Package reference from Controls package #3578
Closed
michael-hawker
michael-hawker previously approved these changes Dec 9, 2020
View reviewed changes
@michael-hawker michael-hawker dismissed their stale review December 9, 2020 00:54

add comment

michael-hawker
michael-hawker approved these changes Dec 9, 2020
View reviewed changes
Copy link
Member

@michael-hawker michael-hawker left a comment

Choose a reason for hiding this comment

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

Thinking I remember why this was done. As now if you press keystrokes multiple times without holding down the Tooltip flashes. Wondering if we could use our Debounce helper around the calls to Visibility.Collapsed for the Tooltip (in the few spots it's called) in order to not have the Tooltip close immediately?

Thoughts?

@Rosuavio
Copy link
Contributor Author

Rosuavio commented Dec 9, 2020

@michael-hawker Looking into it.

Rosuavio
Rosuavio commented Dec 9, 2020
View reviewed changes
Copy link
Contributor Author

@Rosuavio Rosuavio left a comment

Choose a reason for hiding this comment

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

@michael-hawker I tried to use Debounce, feel free to give me some feedback on the implementation.

@Rosuavio Rosuavio added for-review 📖 To evaluate and validate the Issues or PR reviewers wanted ✋ and removed in progress 🚧 labels Dec 10, 2020
Kyaa-dost
Kyaa-dost approved these changes Dec 10, 2020
View reviewed changes
Copy link
Contributor

@Kyaa-dost Kyaa-dost left a comment

Choose a reason for hiding this comment

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

Functions fine without the animation 🚀 🚀

@ghost
Copy link

ghost commented Dec 10, 2020

Hello @michael-hawker!

Because this pull request has the auto merge label, I will be glad to assist with helping to merge this pull request once all check-in policies pass.

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (@msftbot) and give me an instruction to get started! Learn more here.

@michael-hawker michael-hawker merged commit 9e4065f into CommunityToolkit:master Dec 11, 2020
@Rosuavio Rosuavio deleted the range-selector-remove-animation branch December 11, 2020 16:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@michael-hawker michael-hawker michael-hawker approved these changes

@Kyaa-dost Kyaa-dost Kyaa-dost approved these changes

@azchohfi azchohfi Awaiting requested review from azchohfi

Assignees
No one assigned
Labels
animations 🏮 auto merge ⚡ for-review 📖 To evaluate and validate the Issues or PR introduce breaking changes 💥 sample app 🖼
Projects
None yet
Milestone
7.0
Development

Successfully merging this pull request may close these issues.

Remove Animations Package reference from Controls package
4 participants
@Rosuavio @azchohfi @michael-hawker @Kyaa-dost