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

feat: declarativly toggle visibility #64

Merged
merged 1 commit into from
Jul 18, 2021

Conversation

jon-coffey
Copy link

PR Checklist

Please check if your PR fulfills the following requirements:

  • The commit message follows our guidelines: CONTRIBUTING.md#commit
  • Tests for the changes have been added (for bug fixes / features)
  • Docs have been added / updated (for bug fixes / features)

PR Type

What kind of change does this PR introduce?

visibility (by triggering show/hide) can now be done by declarative angular binding ([isVisible]="true|false")

[ ] Bugfix
[x] Feature
[ ] Code style update (formatting, local variables)
[ ] Refactoring (no functional changes, no api changes)
[ ] Build related changes
[ ] CI related changes
[ ] Documentation content changes
[ ] Other... Please describe:

What is the current behavior?

Now show() and hide() have to be called manually.

Issue Number: #60

What is the new behavior?

Now [isVisible]="true|false" can be used to call show()/hide()

Does this PR introduce a breaking change?

[ ] Yes
[x] No

Other information

I tried limit the change to a minimum. Please suggest if something could be done better.

@jon-coffey
Copy link
Author

jon-coffey commented Jul 16, 2021

I mitigated failing test by now using angular-http-server instead of the serve package. that allows fallback to index.html in spa

@jon-coffey
Copy link
Author

Finally. CI is green. Squashed commits look fine. Hope now it's ok. Just tell me if there is more to it.

@@ -105,4 +105,17 @@ describe('@ngneat/helipopper', () => {
.should('exist');
});
});


describe('isVisible', () => {
Copy link
Member

Choose a reason for hiding this comment

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

Please move it to the is visible spec file, and change the file/dir name to dash case

Copy link
Author

Choose a reason for hiding this comment

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

Done

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