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

Allow multiple non-invoking Shortcodes to render #281

Merged
merged 1 commit into from
Nov 26, 2022

Conversation

christianwach
Copy link
Member

Overview

Addresses this issue on Lab

Before

Multiple Afform Shortcodes fail to render.

After

Multiple Afform Shortcodes render correctly.

Comments

The "non-invoking components" e.g. Afform can be extended via the civicrm_no_invoke_shortcode_components filter in case new Afform-like Shortcodes are developed.

@colemanw
Copy link
Member

Thanks a lot for this PR @christianwach.
I actually thought I had this working already as of #244 - can you let me know what I missed in that PR?

@christianwach
Copy link
Member Author

@colemanw Sorry, I should probably have given steps to reproduce. The issue is that multiple Afform Shortcodes fail to render because their associated scripts aren't loaded. Try a page with the following content:

<h3>Afform One</h3>
[civicrm component="afform" name="afsearchOne" hijack="0"]
<h3>Contribution Form One</h3>
[civicrm component="contribution" id="1" action="transact" mode="live" hijack="0"]
<h3>Afform Two</h3>
[civicrm component="afform" name="afsearchTwo" hijack="0"]

Without this PR, I get:

Screenshot 2022-10-17 at 19 47 44

With this PR, everything renders the way I'd expect it to.

@colemanw
Copy link
Member

Makes sense. Code looks fine to me. If someone can test this out I'm happy to merge it.

@christianwach
Copy link
Member Author

@colemanw Thanks for reviewing. I just realised that I missed the "single Afform Shortcode with hijack="1" scenario and that it won't hijack the page/post. I'll push an additional commit tomorrow.

@christianwach
Copy link
Member Author

christianwach commented Oct 18, 2022

I just realised that I missed the "single Afform Shortcode with hijack="1" scenario and that it won't hijack the page/post.

@colemanw So it appears this can't be done because Afforms don't call setTitle() and therefore don't set the global $civicrm_wp_title (yuk). This makes sense, since (allowing for my limited understanding of them) they're not really "pages" as such. Correct me if I'm wrong about this!

I think this PR stands as-is, but definitely needs testing to verify that it supports my list of scenarios:

Single post/page

  • Single invoking Shortcode in single post/page with hijack on and off.
    • Shortcode should render and be functional.
  • Single non-invoking Shortcode in single post/page (hijack not possible).
    • Shortcode should render and be functional.
  • Exactly one invoking Shortcode (with hijack on and off) and one or more non-invoking Shortcodes in single post/page.
    • The invoking Shortcode should render and be functional.
    • The non-invoking Shortcodes should render and be functional.
  • More than one invoking Shortcode (hijack will be ignored) and one or more non-invoking Shortcodes in single post/page.
    • The invoking Shortcodes should render as previews.
    • The non-invoking Shortcodes should render and be functional.

Archives

  • Non-invoking Shortcode(s) should always render and be functional, regardless of how many there are and how many invoking Shortcodes are present - whether in the same post or other posts being displayed.
  • Single invoking Shortcode in single post with hijack on and off where this is the only CiviCRM Shortcode visible in a post archive page.
    • The invoking Shortcode should render and be functional. Hijacking should be respected.
  • One or more invoking Shortcodes in two or more single posts with hijack on and off where these are all visible in a post archive page.
    • The invoking Shortcodes should render as previews. Hijacking should be respected.

And any other scenarios I may have missed, of course. The key thing for testing archives is that as soon as more than one invoking Shortcode is present (whether in the same post or separate posts) then all invoking Shortcodes should be rendered in preview mode.

@colemanw
Copy link
Member

@colemanw So it appears this can't be done because Afforms don't call setTitle()

They do if you load them in standalone mode

@christianwach
Copy link
Member Author

So it appears this can't be done because Afforms don't call setTitle()

They do if you load them in standalone mode

How do I do that?

@colemanw
Copy link
Member

Well, if you're wanting to use invoke() then you could just run CRM_Afform_Page_AfformBase. Or if you want to skip the overhead (since that page doesn't really do anything except load Angular), you could directly load the Angular AfformStandalonePageCtrl like the page does.

@christianwach
Copy link
Member Author

@colemanw Ah right, interesting. It's not something that's directly relevant to this PR though is it? Or am I missing something?

@colemanw
Copy link
Member

@christianwach I was just trying to answer your question about how to get the title to show up

@colemanw
Copy link
Member

@christianwach is this good to merge from your end?

@kcristiano
Copy link
Member

kcristiano commented Nov 26, 2022

@colemanw I've done a good amount of testing and this works. The issues I am having with multiple forms is the markup that afform generates. If we hide the title on WP (fairly common for the use cases I have seen) there is no h1.

It is an improvement, so let's merge and then see what we can do on the markup side.

@kcristiano kcristiano merged commit 8d2fbd2 into civicrm:master Nov 26, 2022
@christianwach christianwach deleted the afform branch March 1, 2023 15:30
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.

3 participants