Skip to content

Feat/new pan view improve animations #1574

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 7 commits into from
Oct 8, 2021
Merged

Feat/new pan view improve animations #1574

merged 7 commits into from
Oct 8, 2021

Conversation

M-i-k-e-l
Copy link
Collaborator

@M-i-k-e-l M-i-k-e-l commented Sep 29, 2021 •
edited
Loading

Description

New PanView - improve animations (using useHiddenLocation).
Note: looking at each commit is easier.
This will have conflicts with #1576

Changelog

New PanView - improve animations (using useHiddenLocation)

@M-i-k-e-l M-i-k-e-l requested a review from ethanshar September 29, 2021 11:41
@M-i-k-e-l M-i-k-e-l mentioned this pull request Sep 30, 2021
Merged
ethanshar
ethanshar reviewed Sep 30, 2021
View reviewed changes
src/incubator/panView/index.tsx Outdated
[dismiss]);

const shouldDismissY = useCallback((isFinished: boolean) => {
const springBack = useCallback(() => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same here regarding the name...
Also why create a method with a similar name to a prop? Don't you find it confusing?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I do find it confusing, any suggestion?

Copy link
Collaborator

@ethanshar ethanshar Oct 4, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

animateToOrigin
animateToInitialPosition

anything that I can read and make sense without diving into the function implementation (:

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

animateToOrigin sounds like a good suggestion to replace animateToStart :)
But calling the function animateToInitialPosition and not animateToOrigin will sound unnatural and forced, don't you think?

Copy link
Collaborator

Choose a reason for hiding this comment

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

animateToOrigin sounds like a good suggestion to replace animateToStart :)

I agree

But calling the function animateToInitialPosition and not animateToOrigin will sound unnatural and forced, don't you think?

Maybe we can have a slight difference in the name, something like animateTargetToOrigin, returnToOrigin, resetToOrigin

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done

@M-i-k-e-l M-i-k-e-l requested a review from ethanshar October 3, 2021 07:06
@ethanshar ethanshar merged commit 6611f22 into master Oct 8, 2021
@M-i-k-e-l M-i-k-e-l deleted the feat/new-pan-view-improve-animations branch October 10, 2021 06:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ethanshar ethanshar ethanshar approved these changes

Assignees

@ethanshar ethanshar

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@M-i-k-e-l @ethanshar