CRM-20304 Bug fixes to Alphabetize options #11045
Conversation
templates/CRM/Custom/Page/Option.tpl
Outdated
| @@ -157,7 +157,7 @@ | |||
| {/literal} | |||
|
|
|||
| <div class="action-link"> | |||
| {crmButton q="reset=1&action=map&fid=$fid&gid=$gid" class="action-item" icon="sort-alpha-asc"}{ts}Alphabetize Options{/ts}{/crmButton} | |||
| {crmButton q="reset=1&action=map&fid=$fid&gid=$gid" class="action-item no-open" icon="sort-alpha-asc"}{ts}Alphabetize Options{/ts}{/crmButton} | |||
There was a problem hiding this comment.
There's already a class for this: no-popup so I don't think you need to create this new one.
There was a problem hiding this comment.
@colemanw I tried using that class, but if I click Alphabetize in a popup window it redirects to the page version. This may be a bug with that class but adding in this new class which is only excluded in the one line below and not in both places exclude is used was the only way I could get around that bug.
js/crm.ajax.js
Outdated
| @@ -498,7 +498,7 @@ | |||
| settings = $el.data('popup-settings') || {}, | |||
| formData = false; | |||
| settings.dialog = settings.dialog || {}; | |||
| if (e.isDefaultPrevented() || !CRM.config.ajaxPopupsEnabled || !url || $el.is(exclude)) { | |||
| if (e.isDefaultPrevented() || !CRM.config.ajaxPopupsEnabled || !url || $el.is(exclude + ', .no-open')) { | |||
There was a problem hiding this comment.
This should not be necessary per my other comment.
There was a problem hiding this comment.
If we want to use no-popup above, we should remove it from the exclude array and instead explicitly exclude it here like I'm doing with no-open. I didn't want to do this since it's used in many places in the code base but in my randomized testing just now it did not seem to be used to close an existing pop up window in the code base (which is what it does in the other place exclude is used).
|
@colemanw Please note my response to your requested changes. Thanks for the review! |
open-inline implies that it will not pop up, so this explicitly excludes them. This allows us to fix the problem where clicking the sort button would add to the history of the popup and take you "back" when clicking done instead of simply closing the dialog.
|
@mepps I looked closer and decided to kill 2 birds with 1 stone by reusing the |
|
@mepps if you can verify my changes then this will be good to go. |
|
@colemanw works fine |
|
Looks good to me, @colemanw. Thanks! |
This builds on yashodha's PR: #10018 and just resolves two bugs.