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

Add metadata to is_primary fields #16113

Merged
merged 1 commit into from
Jan 16, 2020
Merged

Conversation

colemanw
Copy link
Member

Overview

Improves some field information for the sake of Form Builder.

Before

Less accurate field info.

After

Better info about fields for Form Builder to use.

Comments

Strange but true: the Email is_primary field was in the xml twice... and no one noticed for years!

@civibot civibot bot added the master label Dec 17, 2019
@civibot
Copy link

civibot bot commented Dec 17, 2019

(Standard links)

@demeritcowboy
Copy link
Contributor

demeritcowboy commented Dec 17, 2019

Several of the "CheckBox" html type additions are actually Radios on the contact form. I'm not sure if that makes a difference where this data gets used. The confusing one is address location is_primary, which actually is a checkbox. But is_primary for email, IM, open id, phone, etc are all Radio.

@colemanw
Copy link
Member Author

I've thought about it some more and you're right they probably should be radios, but a special type of radio that spans multiple blocks. (e.g. if you have 3 email blocks on a form, the "is_primary" radio between all 3 blocks would be linked so only one can be selected). Still thinking about how to implement that on the Form Builder side. Marking this WIP for now.

@colemanw colemanw changed the title Improve some field metadata WIP Improve some field metadata Dec 17, 2019
@colemanw colemanw changed the title WIP Improve some field metadata Add metadata to is_primary fields Jan 11, 2020
@colemanw colemanw added the merge ready PR will be merged after a few days if there are no objections label Jan 11, 2020
@seamuslee001
Copy link
Contributor

I think this looks right now @demeritcowboy ?

@demeritcowboy
Copy link
Contributor

I'm good.

@eileenmcnaughton
Copy link
Contributor

I'm still a bit icky about the change to radio - if you just showed one field then it would be a checkbox (& on post save the others get changed). I'm not sure if the html type is actually used to display them though so it's possibly ok - @jackrabbithanna does civicrm_entity determine how to display fields based on html_type?

@colemanw
Copy link
Member Author

colemanw commented Jan 16, 2020

@eileenmcnaughton yes I struggled with this too, but what eventually made the icky taste go away was considering:

  1. This isn't a change since there's no metadata currently.
  2. On civi forms they are already radios.
  3. Checkboxes really doesn't make sense if you think about it. What is unchecking a box supposed to mean? You can't not have a primary.
  4. This works great for afforms :)

@eileenmcnaughton
Copy link
Contributor

@colemanw well afform is the way of the future .... OK

@eileenmcnaughton eileenmcnaughton merged commit 70fe0a4 into civicrm:master Jan 16, 2020
@eileenmcnaughton eileenmcnaughton deleted the meta branch January 16, 2020 03:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
master merge ready PR will be merged after a few days if there are no objections
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants