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

fix(toast): isVisible property binding - master #8775

Merged
merged 25 commits into from
Jan 20, 2021

Conversation

dobromirts
Copy link
Contributor

@dobromirts dobromirts commented Jan 12, 2021

Closes #8207

Additional information (check all that apply):

  • Bug fix
  • New functionality
  • Documentation
  • Demos
  • CI/CD

Checklist:

  • All relevant tags have been applied to this PR
  • This PR includes unit tests covering all the new code (test guidelines)
  • This PR includes API docs for newly added methods/properties (api docs guidelines)
  • This PR includes feature/README.MD updates for the feature docs
  • This PR includes general feature table updates in the root README.MD
  • This PR includes CHANGELOG.MD updates for newly added functionality
  • This PR contains breaking changes
  • This PR includes ng update migrations for the breaking changes (migrations guidelines)
  • This PR includes behavioral changes and the feature specification has been updated with them

@hanastasov hanastasov added 💥 status: in-test PRs currently being tested and removed ❌ status: awaiting-test PRs awaiting manual verification labels Jan 13, 2021
Copy link
Contributor

@hanastasov hanastasov left a comment

Choose a reason for hiding this comment

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

Please add a test that verifies setting isVisible actually opens/shows the toast.

Copy link
Contributor

@hanastasov hanastasov left a comment

Choose a reason for hiding this comment

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

Using event handlers for executing expectations results in false negative test - if event handlers are not called, test will still succeed, while there is a bug (toast is not actually opened and event is not emitted).
It's better to use jasmine spyOn to test if event is emitted.

Also, after discussion, we agreed for the following enhacements:

  1. Rethinkg igxToggleDircetive open method:
        this.collapsed = false;
        this.cdr.detectChanges();

 

        const openEventArgs: ToggleViewCancelableEventArgs = { cancel: false, owner: this, id: this._overlayId };
        this.onOpening.emit(openEventArgs);
        if (openEventArgs.cancel) {
            this.collapsed = true;
            this.cdr.detectChanges();
            return;
        }

Seems too much. Must be safe to emit the onOpening event first, and if canceled, then return. Everything else to be removed.

  1. Rethink if this.unsubscribe() is needed
    All those subscriptions (_overlayAppendedSub, _overlayOpenedSub, _overlayClosingSub, _overlayClosedSub) can be refactored to take the first emitted value only:
        this._overlayOpenedSub = this.overlayService.onOpened.pipe(...this._overlaySubFilter, first())...
  1. Rethink if safe to move all subscriptions (_overlayAppendedSub, _overlayOpenedSub, _overlayClosingSub, _overlayClosedSub) in the constructor. Make sure _overlayId is correct.

  2. See if any detectChanges() call is excessive.Try to move all of those in the overlay.

  3. Deprecate toast showing and hiding events, as suggested in the issue.

zdrawku
zdrawku previously approved these changes Jan 19, 2021
Copy link
Contributor

@zdrawku zdrawku left a comment

Choose a reason for hiding this comment

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

Well done. I see that the changes regarding the migration are applied as well.

Copy link
Contributor

@hanastasov hanastasov left a comment

Choose a reason for hiding this comment

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

  1. visibility is updated by the toggle() method
  2. should open and close toast when set values to isVisible
  3. should emit onHidden when toggle onClosed is fired
  4. should emit onShown when toggle onOpened is fired

The above tests implementation result in false negative tests. Please refactor those.

@zdrawku zdrawku added ✅ status: verified Applies to PRs that have passed manual verification and removed 💥 status: in-test PRs currently being tested labels Jan 20, 2021
@zdrawku zdrawku merged commit 0921a1b into master Jan 20, 2021
@zdrawku zdrawku deleted the dTsvetkov/fix-8207-master branch January 20, 2021 13:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
toast version: 11.1.x ✅ status: verified Applies to PRs that have passed manual verification
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Setting isVisible does not show the toast
3 participants