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

CRM-21840: Show Options Edit Link for Radio and Checkbox Groups #11819

Merged

Conversation

mickadoo
Copy link
Contributor

@mickadoo mickadoo commented Mar 15, 2018

Overview

A tool icon is provided to allow editing of option groups, however this icon is only shown for select input types.

To make it easier to edit option groups this PR will also display the edit link for radio and checkbox input types.

Before

The edit link is only shown for select input types

image

image

After

The edit link is also shown for radio groups.

image

The same is true for radio options belonging to custom fields

image

It is also shown for checkbox groups.

Radio button edit example

peek 2018-03-19 15-29

Checkbox group edit example

peek 2018-03-20 12-58


@colemanw
Copy link
Member

@mickadoo the reason that only <select> elements were originally supported is because the javascript has to change the form element markup after the popup is saved. That's much easier to do with a select list. If you want to try tackling it, that code is here:
https://github.com/civicrm/civicrm-core/blob/master/js/crm.optionEdit.js

@mickadoo
Copy link
Contributor Author

@colemanw ah right, that makes sense. I've added a few changes to support refreshing radio options after saving. I'm no whiz with Javascript so prepare yourself. I've also added a GIF to the PR description showing it in action.

@colemanw
Copy link
Member

I'd suggest not putting that code in Common.js since it is not needed elsewhere. Just leave it in the crm.optionEdit.js file since that's the only place it's used.
Also, if we're bothering to do this for radios, the code should be identical for checkboxes.

@mickadoo mickadoo changed the title CRM-21840: Show Options Edit Link for Radio Groups CRM-21840: Show Options Edit Link for Radio and Checkbox Groups Mar 20, 2018
@mickadoo
Copy link
Contributor Author

@colemanw I've added support for checkboxes, updated the PR description and added another gif showing checkbox options editing.

@colemanw
Copy link
Member

This looks great @mickadoo - thanks for the screencasts.
My one nitpick is that the js code could be more concise. The functions refreshSelectOptions refreshRadioOptions and refreshCheckboxOptions are identical except for one line, so that could be a param to a consolidated fn.

@mickadoo
Copy link
Contributor Author

mickadoo commented Mar 20, 2018

@colemanw I did some refactoring in the last commit to remove duplicate code.

I noticed that non-custom checkbox inputs do not have a hidden input:

<input 
  type="checkbox"
  id="preferred_communication_method_1" 
  name="preferred_communication_method[1]" 
  value="1" 
  class="crm-form-checkbox" 
>

But custom fields checkbox options all seem to have a hidden element with duplicate name, and no value:

<input type="hidden" name="custom_7_1[1]" value="">
<input
  type="checkbox"
  id="custom_7_1_1"
  name="custom_7_1[1]"
  value="1"
  class="crm-form-checkbox"
>

Any idea why the hidden element exists? I've tested without it and it doesn't seem to make any difference to saving the values.

@colemanw
Copy link
Member

@mickadoo final task in this PR would be to rebase & squash it into one commit. That keeps the commit history on the master branch clean, minus the WIP back & forth commits in the PR.

@mickadoo mickadoo force-pushed the CRM-21840-show-tool-icon-for-radio-groups branch from e219d4d to 3ef9334 Compare March 20, 2018 16:38
@mickadoo
Copy link
Contributor Author

@colemanw squashed into one commit, there was one civilint warning before this but hopefully that should be fixed now too.

@colemanw
Copy link
Member

I've reviewed the code and tested this.

@colemanw colemanw merged commit d69014e into civicrm:master Mar 20, 2018
@mickadoo mickadoo deleted the CRM-21840-show-tool-icon-for-radio-groups branch March 21, 2018 09:32
mickadoo added a commit to compucorp/civicrm-core that referenced this pull request Apr 5, 2018
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.

3 participants