Skip to content

fix hint animation #1363

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

Merged
merged 11 commits into from
Jul 4, 2021
Merged

fix hint animation #1363

merged 11 commits into from
Jul 4, 2021

Conversation

mendyEdri
Copy link
Contributor

Description

Fix missing Hint bubble fade-out animation

Changelog

Added missing hide animation for Hint bubble

Copy link
Collaborator

@ethanshar ethanshar left a comment

Choose a reason for hiding this comment

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

@mendyEdri
Android throws an error

@mendyEdri mendyEdri requested a review from ethanshar June 20, 2021 14:58
Copy link
Collaborator

@ethanshar ethanshar left a comment

Choose a reason for hiding this comment

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

@mendyEdri
Android doesn't throw an error now, but also doesn't work.
See video

Also, check my comment, I don't think that's the right fix (:

Screen.Recording.2021-06-21.at.8.09.46.mov

@mendyEdri mendyEdri requested a review from ethanshar June 27, 2021 13:48
if (!visible) {
const {onBackgroundPress, testID} = this.props;
if (!this.props.visible && this.state.animationEnded) {
console.log('removed');
Copy link
Collaborator

Choose a reason for hiding this comment

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

You forgot a console.log (:

Comment on lines 194 to 199
this.setState({animationEnded: true}, () => {
setTimeout(() => {
this.setState({animationEnded: false});
}, this.animationDuration);
});
};
Copy link
Collaborator

Choose a reason for hiding this comment

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

This forces us to trigger two setState calls every time we trigger the animation. twice on enter twice on exit.
Maybe we can reduce that..
If you think about it, while the Hint is visible it doesn't matter really what's the value of animationEnded state.

What if instead of keeping animationEnded you'll keep something else, like actuallyVisible, hasUnmounted (or anything with a better name (: )
Then you'll update it once on enter and once on exit...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Right, fixed

@mendyEdri mendyEdri requested a review from ethanshar July 4, 2021 08:57
@ethanshar ethanshar merged commit f2c4339 into master Jul 4, 2021
M-i-k-e-l added a commit that referenced this pull request Jul 13, 2021
* master:
  Upgrade eslint-plugin-uilib to version 2.0.09
  Typescript/wizard move to typescript (#1390)
  Feat/hint add props (#1399)
  Separate radioGroup and radioButton to fix their packages (#1388)
  Typescript/connection status bar (#1384)
  Fix typescript migration bug, code should enter here to calcylate tip position also when width is undefined (casting to 0) (#1396)
  Fix an exception with hard coded color rule (#1394)
  Add onChange to dependency array (#1391)
  fix colorPicker ts error
  Update stale.yml
  Typescript/general missing typings (#1389)
  fix hint animation (#1363)
  using renderInput instead renderExpandableInput was left out (#1386)
  DateTimePicker - Implement theme variant prop (#1357)
  Infra/Migrate Incubator.WheelPicker to reanimated 2 (#1379)
  Remove old typings that were migrated to generatedTypes
  Fix Hint export in generatedType/index
  Update react-native-gesture-handler to version 1.10.3
  Fix export of AvatarHelper in src/index.ts (#1376)
  ExampleScreenPresenter - fix typo (#1374)
@mendyEdri mendyEdri deleted the fix/hint-animation branch July 26, 2021 08:00
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.

2 participants