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

Issue#483 #551

Closed
wants to merge 2 commits into from
Closed

Issue#483 #551

wants to merge 2 commits into from

Conversation

AmooHashem
Copy link

As was mentioned in issue comments, I search for a race condition and check every thing that thought might have effect on race.
I just change a setTimeout() duration to zero and it works (at least in playground) and also, it pass all the tests.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 93.596% when pulling 5783058 on AmooHashem:issue#483 into eac7f7b on fkhadra:master.

@fkhadra
Copy link
Owner

fkhadra commented Dec 20, 2020

Hey @AmooHashem, thanks for your contribution.

Unfortunately, this change will prevent the collapse animation to work when you have many notifications displayed at the same time.

Screen.Recording.2020-12-20.at.8.42.49.PM.mov

@fkhadra
Copy link
Owner

fkhadra commented Dec 20, 2020

@AmooHashem, one solution would be maybe to do as follow

export function collapseToast(
  node: HTMLElement,
  done: () => void,
  duration = DEFAULT.COLLAPSE_DURATION
) {
  const height = node.scrollHeight;
  const style = node.style;
  
  // rely  on transitionend instead of timeout 
  node.addEventListener('transitionend', () => done());
  
  requestAnimationFrame(() => {
    style.minHeight = 'initial';
    style.height = height + 'px';
    style.transition = `all ${duration}ms`;

    requestAnimationFrame(() => {
      style.height = '0';
      style.padding = '0';
      style.margin = '0';
      // remove setTimeout
      // setTimeout(() => done(), duration as number);
    });
  });
}

@AmooHashem
Copy link
Author

AmooHashem commented Dec 20, 2020

@AmooHashem, one solution would be maybe to do as follow

export function collapseToast(
  node: HTMLElement,
  done: () => void,
  duration = DEFAULT.COLLAPSE_DURATION
) {
  const height = node.scrollHeight;
  const style = node.style;
  
  // rely  on transitionend instead of timeout 
  node.addEventListener('transitionend', () => done());
  
  requestAnimationFrame(() => {
    style.minHeight = 'initial';
    style.height = height + 'px';
    style.transition = `all ${duration}ms`;

    requestAnimationFrame(() => {
      style.height = '0';
      style.padding = '0';
      style.margin = '0';
      // remove setTimeout
      // setTimeout(() => done(), duration as number);
    });
  });
}

but in this case, it will fail some tests. I also checked it.

@fkhadra
Copy link
Owner

fkhadra commented Dec 20, 2020

I'm currently working on the removal of react-transition-group #514 . I have a working solution but I have to adapt a bunch of tests right now. There is an impact on this part of the code. I'll try to push my code when it's stable

@fkhadra
Copy link
Owner

fkhadra commented Dec 29, 2020

I found the root cause. I'll open a PR soon with the fix

@fkhadra fkhadra closed this Dec 29, 2020
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.

3 participants