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

Remove legacy template field #322

Closed
wants to merge 1 commit into from

Conversation

agileware-pengyi
Copy link

The legacy mailing template is not used in Mosaico template, however, the field is shown when editing Moscaico mailing without shoreditch. This cause users to confuse.

Before

Legacy template field in the form.

After

Legacy template field removed from the form.

Comment

Feel free to improve this PR since I am not familiar with Angular.

Agileware ref: CIVIMOSAIC-18

@eileenmcnaughton
Copy link
Collaborator

@agileware-pengyi just very superficially - the description makes me think you would be REMOVING code but you are adding a lot - which makes me wonder why... Perhaps screenshots would help

@agileware-pengyi
Copy link
Author

@eileenmcnaughton the removed line reusing this file. I replaced the line with its content and remove the template field. So the field will not be displayed in Mosaico editor.
mailing_block

@seamuslee001
Copy link
Contributor

ok so looking at this the concept of the PR makes 100% sense to me, the way the PR has been done doesn't 100% make as much sense to me but that is ok. Just verballing out there are possibly other options we could pursuse

  1. Alter CiviCRM-Core's BlockMailing.html to do an ng-if on the template if the type is not traditional
  2. We could create our own new directive here and specifiy it like the block Mailing, This would potentially have the advantage of having a structure that is similar to the base stuff so developers may understand things easier
  3. Do as this PR is doing which is bringing the code we want from the BlockMailing directive into the main file here

@totten @eileenmcnaughton @agileware-pengyi i'm not sure what is overall the best option here but my thinking would be 1 would be the most simple to do and possibly have the best results overall for any future template builder (may not necessairly be Mosacio).

@eileenmcnaughton
Copy link
Collaborator

1 seems Ok to me - but I'm less familiar with this code

@agileware-pengyi
Copy link
Author

I would prefer 1 if the traditional templates won't be used on any other editors.

@seamuslee001
Copy link
Contributor

I would assume that traditional templates wouldn't be used at all going forward for newer editiors. I would appreciate @totten weighing in as well

@totten
Copy link
Collaborator

totten commented Jul 29, 2019

Agree, it's really nice how #1 avoids duplicating the code. The essential logic also sounds valid: "content from civicrm_msg_template can be used if-and-only-if the mailing has template_type==traditional (because the GUI for editing civicrm_msg_template is geared toward traditional)"

In the long-term, it might be ideal for civicrm_msg_template to also have a template_type (and bunch of other changes to make that workable). However, in the foreseeable future, I think #1 is a straight-forward improvement.

@totten
Copy link
Collaborator

totten commented Jul 29, 2019

Feel free to improve this PR since I am not familiar with Angular.

@agileware-pengyi - Just want to say - for someone coming in new to Angular, it looks like a good start. 👍

I know more feedback here becomes moot when changing to #1, but it may be useful to follow-through with a little feedback anyway. Consider these files:

  • mosaico - ang/crmMosaico.crmstar/EditMailingCtrl/crmstar-single.html
  • mosaico - ang/crmMosaico.bootstrap/EditMailingCtrl/bootstrap-single.html
  • mosaico - ang/crmMosaico.bootstrap/EditMailingCtrl/bootstrap-wizard.html
  • core - ang/crmMailing/EditMailingCtrl/2step.html
  • core - ang/crmMailing/EditMailingCtrl/unified.html
  • core - ang/crmMailing/EditMailingCtrl/unified2.html
  • core - ang/crmMailing/EditMailingCtrl/wizard.html
  • core - ang/crmMailing/EditMailingCtrl/workflow.html

All of these files are basically the same, except that they remix the various UI elements in different layouts. The current set of directives (crm-mailing-block-summary, crm-mailing-block-mailing, crm-mailing-block-preview, etc) was meant to facilitate this.

The change here (hiding one item from crm-mailing-block-mailing in the context of Mosaico) is not easy with the current set of blocks/options - they just don't support that nicely. The approach in 322 (i.e. inlining the block completely) is understandable but feels a bit heavy-handed. Both Seamus's #1 and #2 feel better than #3 because they still support some remixability.

There is a good argument that maybe the contract for these directives (crm-mailing-block-summary etc) should be made for flexible. I don't see a perfect alternative contract for all possible scenarios off-the-cuff, but this seems like an example of a better one: crm-mailing-ab-block-mailing="{field:bool, field:bool, ...}")

https://github.com/civicrm/civicrm-core/blob/7cded6f/ang/crmMailingAB/EditCtrl/edit.html#L33-L51

That allows all of the wonky field-markup to be defined one time (crmMosaicoAB/BlockMailing.html) while also allowing elements to be rearranged/cherry-picked.

@jusfreeman
Copy link

jusfreeman commented Jul 31, 2019

Just noting that alternative PR has been submitted here civicrm/civicrm-core#14927

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants