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

(Recommendation) Prefer $uibModalInstance.close() to dismiss modals #506

Closed
jniles opened this issue Jun 17, 2016 · 0 comments
Closed

(Recommendation) Prefer $uibModalInstance.close() to dismiss modals #506

jniles opened this issue Jun 17, 2016 · 0 comments

Comments

@jniles
Copy link
Collaborator

jniles commented Jun 17, 2016

This recommendation is based off a conversation with @DedrickEnc, who brought it to my attention.

The normal method for canceling or dismissing modals is to use the $uibModalInstance.dismiss() call. Unfortunately, this will reject the modal's promise instance, causing the .catch() method on the promise to fire.

A better way would be to call close with no data (or with false), and exit out of the promise chain that way. See below:

var modal = Modals.openSomeModal();

modal.then(function (data) {

  // if we call $uibModalInstance.close(), or $uibModalInstance.close(false), we can 
  // use the 'if' statement below to exit out of this promise statement before 
  // any other logic occurs.
  if (!data) { return; }

  // this will never fire if data is false-y
  return $http.post('/some/url', data);
})
.catch(function (err) {

  // if we call $uibModalInstance.dismiss() inside the modal, we will end 
  // up in this catch statement.  This is bad if this promise chain 
  // also involves a HTTP request, or uses Notify.handleError()
  // to automatically try and display an error to the user!

  Notify.handleError(err);  // oh-no! `err` is undefined!
});
jniles referenced this issue in jniles/bhima Jun 17, 2016
This commit improves the filters ui again by introducing the following
upgrades:
 1. The server uses `moment` to parse dates in the date filters.  The
 code on the server for rendering the filters is very similar to the
 clientside codebase, which is unavoidable.  There might be an
 optimisation down the road to reduce this replication, but I think this
 is acceptable for now.

 2. The filters are now passed into the filter form and preset there.
 That way, when you go to filter a second time, all the data is
 preserved.

 3. A weird bug with caching filters was fixed.  The detailed flag no
 longer shows up as a potential filter, and refreshing the page after
 printing no longer breaks the browser.

 4. The date rendering has been improved in the handlebar's date helper
 function.  If an invalid date occurs, it will simply render the empty
 string.

 5. The `$uibModal` handler now implements the behavior described in
 #506.

Closes #492.  Closes #336.
jniles referenced this issue in jniles/bhima Jun 21, 2016
This commit improves the filters ui again by introducing the following
upgrades:
 1. The server uses `moment` to parse dates in the date filters.  The
 code on the server for rendering the filters is very similar to the
 clientside codebase, which is unavoidable.  There might be an
 optimisation down the road to reduce this replication, but I think this
 is acceptable for now.

 2. The filters are now passed into the filter form and preset there.
 That way, when you go to filter a second time, all the data is
 preserved.

 3. A weird bug with caching filters was fixed.  The detailed flag no
 longer shows up as a potential filter, and refreshing the page after
 printing no longer breaks the browser.

 4. The date rendering has been improved in the handlebar's date helper
 function.  If an invalid date occurs, it will simply render the empty
 string.

 5. The `$uibModal` handler now implements the behavior described in
 #506.

Closes #492.  Closes #336.
@jniles jniles closed this as completed Jul 17, 2017
bors bot added a commit that referenced this issue Oct 1, 2018
3212: Update commitizen to the latest version 🚀 r=jniles a=greenkeeper[bot]


## The devDependency [commitizen](https://github.com/commitizen/cz-cli) was updated from `2.10.1` to `3.0.0`.
This version is **not covered** by your **current version range**.

If you don’t accept this pull request, your project will work just like it did before. However, you might be missing out on a bunch of new features, fixes and/or performance improvements from the dependency update.

---

<details>
<summary>Release Notes for v3.0.0</summary>

<p>&lt;a name"3.0.0"&gt;</p>
<h2>3.0.0 (2018-10-01)</h2>
<h4>Bug Fixes</h4>
<ul>
<li>resolve linux build issues, add git exit code 128 warning (<a href="https://urls.greenkeeper.io/commitizen/cz-cli/commit/c3764c8c">c3764c8c</a>)</li>
<li><strong>deps:</strong>
<ul>
<li>update dependency glob to v7.1.2 (<a class="issue-link js-issue-link" data-error-text="Failed to load issue title" data-id="326327664" data-permission-text="Issue title is private" data-url="commitizen/cz-cli#506" href="https://urls.greenkeeper.io/commitizen/cz-cli/pull/506">#506</a>) (<a href="https://urls.greenkeeper.io/commitizen/cz-cli/commit/aa824960">aa824960</a>)</li>
<li>update dependency lodash to v4.17.10 (<a class="issue-link js-issue-link" data-error-text="Failed to load issue title" data-id="326338773" data-permission-text="Issue title is private" data-url="commitizen/cz-cli#507" href="https://urls.greenkeeper.io/commitizen/cz-cli/pull/507">#507</a>) (<a href="https://urls.greenkeeper.io/commitizen/cz-cli/commit/4bda9435">4bda9435</a>)</li>
<li>update dependency find-root to v1.1.0 (<a class="issue-link js-issue-link" data-error-text="Failed to load issue title" data-id="326327598" data-permission-text="Issue title is private" data-url="commitizen/cz-cli#505" href="https://urls.greenkeeper.io/commitizen/cz-cli/pull/505">#505</a>) (<a href="https://urls.greenkeeper.io/commitizen/cz-cli/commit/67a4e16a">67a4e16a</a>)</li>
<li>update dependency dedent to v0.7.0 (<a class="issue-link js-issue-link" data-error-text="Failed to load issue title" data-id="326314146" data-permission-text="Issue title is private" data-url="commitizen/cz-cli#504" href="https://urls.greenkeeper.io/commitizen/cz-cli/pull/504">#504</a>) (<a href="https://urls.greenkeeper.io/commitizen/cz-cli/commit/a21b674f">a21b674f</a>)</li>
<li>update dependency cz-conventional-changelog to v2.1.0 (<a class="issue-link js-issue-link" data-error-text="Failed to load issue title" data-id="326314071" data-permission-text="Issue title is private" data-url="commitizen/cz-cli#503" href="https://urls.greenkeeper.io/commitizen/cz-cli/pull/503">#503</a>) (<a href="https://urls.greenkeeper.io/commitizen/cz-cli/commit/43d54de1">43d54de1</a>)</li>
</ul>
</li>
<li><strong>package:</strong> Remove opencollective post-install hook (<a class="issue-link js-issue-link" data-error-text="Failed to load issue title" data-id="356422024" data-permission-text="Issue title is private" data-url="commitizen/cz-cli#561" href="https://urls.greenkeeper.io/commitizen/cz-cli/pull/561">#561</a>) (<a href="https://urls.greenkeeper.io/commitizen/cz-cli/commit/a70c234d">a70c234d</a>)</li>
</ul>
<h4>Features</h4>
<ul>
<li>Drop support for Node.js &lt;6.x, update dependencies (<a class="issue-link js-issue-link" data-error-text="Failed to load issue title" data-id="361350875" data-permission-text="Issue title is private" data-url="commitizen/cz-cli#566" href="https://urls.greenkeeper.io/commitizen/cz-cli/pull/566">#566</a>) (<a href="https://urls.greenkeeper.io/commitizen/cz-cli/commit/a70c063b">a70c063b</a>)</li>
<li>support initialization with yarn. (<a class="issue-link js-issue-link" data-error-text="Failed to load issue title" data-id="349367370" data-permission-text="Issue title is private" data-url="commitizen/cz-cli#549" href="https://urls.greenkeeper.io/commitizen/cz-cli/pull/549">#549</a>) (<a href="https://urls.greenkeeper.io/commitizen/cz-cli/commit/d5652154">d5652154</a>, closes <a href="https://urls.greenkeeper.io/commitizen/cz-cli/issues/527">#527</a>, <a href="https://urls.greenkeeper.io/commitizen/cz-cli/issues/527">#527</a>)</li>
</ul>
<h4>Breaking Changes</h4>
<ul>
<li>Older versions of Node.js are no longer supported</li>
</ul>
<p>Includes most updates with the exception of semantic-release which breaks on windows. To be investigated in a later release.<br>
(<a href="https://urls.greenkeeper.io/commitizen/cz-cli/commit/a70c063b">a70c063b</a>)</p>
</details>

<details>
<summary>Commits</summary>
<p>The new version differs by 23 commits.</p>
<ul>
<li><a href="https://urls.greenkeeper.io/commitizen/cz-cli/commit/a70c063b06dbdf41af322dab06f83bfddd69149b"><code>a70c063</code></a> <code>feat: Drop support for Node.js &lt;6.x, update dependencies (#566)</code></li>
<li><a href="https://urls.greenkeeper.io/commitizen/cz-cli/commit/0d76887b7853f673949cf7aee04653b90b4dda7b"><code>0d76887</code></a> <code>chore: remove sign from windows build</code></li>
<li><a href="https://urls.greenkeeper.io/commitizen/cz-cli/commit/1546df4856dc8c4aa781e9e0ea755613647642ca"><code>1546df4</code></a> <code>chore: assume yarn in tests</code></li>
<li><a href="https://urls.greenkeeper.io/commitizen/cz-cli/commit/c3764c8c1d2b05597a6c604c13b6a11731c47281"><code>c3764c8</code></a> <code>fix: resolve linux build issues, add git exit code 128 warning</code></li>
<li><a href="https://urls.greenkeeper.io/commitizen/cz-cli/commit/f1150c83a62048831fe6d94836f52242fdc9831e"><code>f1150c8</code></a> <code>chore: Create jobs build yml</code></li>
<li><a href="https://urls.greenkeeper.io/commitizen/cz-cli/commit/f0595edca921b68913e2ce2ca1bbad75abe71515"><code>f0595ed</code></a> <code>chore: add azure jobs</code></li>
<li><a href="https://urls.greenkeeper.io/commitizen/cz-cli/commit/4693079d0400ad1a914af86c6027f83d18cde74d"><code>4693079</code></a> <code>chore: Set up CI with Azure Pipelines</code></li>
<li><a href="https://urls.greenkeeper.io/commitizen/cz-cli/commit/d5652154fa1bb41c1b1c84f20c620ee64ca32c78"><code>d565215</code></a> <code>feat: support initialization with yarn. fixes #527 (#549)</code></li>
<li><a href="https://urls.greenkeeper.io/commitizen/cz-cli/commit/a70c234d8471af147cc9f7d9d090b6ba2192eb17"><code>a70c234</code></a> <code>fix(package): Remove opencollective post-install hook (#561)</code></li>
<li><a href="https://urls.greenkeeper.io/commitizen/cz-cli/commit/d5b8bc587770e4a7efd6a36962abaa0425931181"><code>d5b8bc5</code></a> <code>docs(readme): add npx use examples (#532)</code></li>
<li><a href="https://urls.greenkeeper.io/commitizen/cz-cli/commit/d103b10f56121a8617ac9ac882338338417b0947"><code>d103b10</code></a> <code>chore(ci): test on Node.js 10.x (#531)</code></li>
<li><a href="https://urls.greenkeeper.io/commitizen/cz-cli/commit/41a921b002d1627cfedd4bb5745a27d8a47a5dfe"><code>41a921b</code></a> <code>BREAKING CHANGE: Remove Node 0.12 support (#524)</code></li>
<li><a href="https://urls.greenkeeper.io/commitizen/cz-cli/commit/aa8249605d45c1e95f57be7c832321212b0e8d1c"><code>aa82496</code></a> <code>fix(deps): update dependency glob to v7.1.2 (#506)</code></li>
<li><a href="https://urls.greenkeeper.io/commitizen/cz-cli/commit/4bda94350625e54ca862cd143432de1912caea27"><code>4bda943</code></a> <code>fix(deps): update dependency lodash to v4.17.10 (#507)</code></li>
<li><a href="https://urls.greenkeeper.io/commitizen/cz-cli/commit/67a4e16a88c7925f3199996002ba8da37e420cff"><code>67a4e16</code></a> <code>fix(deps): update dependency find-root to v1.1.0 (#505)</code></li>
</ul>
<p>There are 23 commits in total.</p>
<p>See the <a href="https://urls.greenkeeper.io/commitizen/cz-cli/compare/5838ce007c1af2e71ef6ae68efce7682d59e9061...a70c063b06dbdf41af322dab06f83bfddd69149b">full diff</a></p>
</details>

<details>
  <summary>FAQ and help</summary>

  There is a collection of [frequently asked questions](https://greenkeeper.io/faq.html). If those don’t help, you can always [ask the humans behind Greenkeeper](https://github.com/greenkeeperio/greenkeeper/issues/new).
</details>

---


Your [Greenkeeper](https://greenkeeper.io) bot 🌴



Co-authored-by: greenkeeper[bot] <greenkeeper[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

1 participant