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

Towards using EntityForm.tpl for Membership type & enabling custom data #12591

Merged
merged 3 commits into from
Jul 30, 2018

Conversation

eileenmcnaughton
Copy link
Contributor

Overview

We now have a standard for enabling custom data on a form that does not currently support it - ie. by using the EntityForm.tpl. The membership type form is partially converted & this takes that a bit further

Before

No UI change. Less usage of metadata

After

No UI change, more usage of metadata

Technical Details

There are 2 things to progress to switch the form over

  1. removing fieldAdd type stuff from buildForm
  2. removing field specific stuff from the tpl

This does a bit of both. It's mostly aimed at reducing some of the 'noise' to find the real issues in the conversion. So far I have spotted the auto-renew text might be a problem. I think the metadata will handle it as written but I haven't tested much and there is nothing in Field.tpl to cope with putting 2 fields on the same row which is the next challenge.

  • note the js can stay in the tpl after the entity form inclusion - this slight change in html structure should not impact the js as it uses field ids not html markup.

Comments

@mattwire do you want to take a look - it's just another incremental step & a way to break this down into more manageable chunks

@civibot
Copy link

civibot bot commented Jul 29, 2018

(Standard links)

Note that doing a partial conversion in the template is just a stepping stone to using the
EntityField template, when lots of fields need conversion / review it allows us to break it up

Simplify converstion of fields, convert 2 more
@eileenmcnaughton
Copy link
Contributor Author

Just a quick note that I tested the l10n part - before & after both like
screenshot 2018-07-30 10 58 59

@mattwire do you want to check this & we can get this merged & then look at 'the next field off the block' - which is autorenew. I haven't done before & afters on that field without a processor enabled yet

@mattwire
Copy link
Contributor

@mattwire
Copy link
Contributor

Ok to merge @seamuslee001 @eileenmcnaughton

@eileenmcnaughton eileenmcnaughton merged commit 771c671 into civicrm:master Jul 30, 2018
@eileenmcnaughton eileenmcnaughton deleted the membership_type branch July 30, 2018 20:03
@eileenmcnaughton
Copy link
Contributor Author

thanks @mattwire

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