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(modal): add DDAU for modal visibility #140

Merged
merged 1 commit into from May 14, 2019
Merged

fix(modal): add DDAU for modal visibility #140

merged 1 commit into from May 14, 2019

Conversation

fkm
Copy link
Contributor

@fkm fkm commented May 13, 2019

No description provided.

@fkm fkm requested a review from czosel May 13, 2019 15:42
@@ -4,6 +4,8 @@
{{#uk-modal
visible=visible
on-submit=(action "submit")
on-show=(action (mut visible) true)
Copy link
Contributor

Choose a reason for hiding this comment

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

on-hide makes sense to me because of DDAU, but on-show looks super unnecessary: The "hide" event can be triggered from the modal, but the "show" event can't. I'd say the default implementation for show should remain in the component, and on-show would then be an extremely optional hook that we'd probably need only for very special use cases.

The public API would then be:

{{#uk-modal
  visible=visible # component reacts to outside state of "visible"
  on-hide=(...)
}} 

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If I'm not mistaken, the modal can still be opened via a button. For that case we should have the possibility to update our toggle variable.

Copy link
Contributor

Choose a reason for hiding this comment

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

Right. But let's not force the user to specify on-show, because with the documented usage it's redundant.

@czosel
Copy link
Contributor

czosel commented May 14, 2019

@anehx I think we shouldn't force our users to implement on-show - do you agree?

Currently, the modal won't open if one doesn't implement it.

@fkm
Copy link
Contributor Author

fkm commented May 14, 2019

@czosel Did you test that? Works fine in my test environment.

Copy link
Contributor

@czosel czosel left a comment

Choose a reason for hiding this comment

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

Sorry, I got quite confused about the visible property - i was thinking that on-show is a mandatory argument now. All good 👍

@czosel czosel merged commit 6a3ddef into adfinis:master May 14, 2019
czosel pushed a commit that referenced this pull request Jun 11, 2019
## [0.8.1](v0.8.0...v0.8.1) (2019-06-11)

### Bug Fixes

* **deps:** update dependencies and drop ember-cli-eslint ([1a6023d](1a6023d))
* **deps:** update ember source ([3d293ba](3d293ba))
* **modal:** add DDAU for modal visibility ([#140](#140)) ([6a3ddef](6a3ddef))
* **modal:** reintroduce option for close button ([8a719e9](8a719e9))
* **notification:** wrong property in snippet ([f925d47](f925d47))
* **syntax:** use the new template syntax ([142aa3a](142aa3a))
* **tests:** fix wrong attribute in uk-toggle-switch test ([73be406](73be406))
czosel pushed a commit that referenced this pull request Jun 11, 2019
## [0.8.2](v0.8.1...v0.8.2) (2019-06-11)

### Bug Fixes

* **deps:** update dependencies and drop ember-cli-eslint ([1a6023d](1a6023d))
* **deps:** update ember source ([3d293ba](3d293ba))
* **modal:** add DDAU for modal visibility ([#140](#140)) ([6a3ddef](6a3ddef))
* **notification:** wrong property in snippet ([f925d47](f925d47))
* **syntax:** use the new template syntax ([142aa3a](142aa3a))
* **tests:** fix wrong attribute in uk-toggle-switch test ([73be406](73be406))
@czosel
Copy link
Contributor

czosel commented Jun 11, 2019

🎉 This PR is included in version 0.8.2 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants