-
-
Notifications
You must be signed in to change notification settings - Fork 825
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
Afform - Remove ngRoute from afformStandalone page #19703
Conversation
(Standard links)
|
A couple questions/thoughts:
|
Hmm, there ought to be a way to to handle query strings without needing a full router. I'll take another look at that. |
2 test fails relate api_v4_AfformRoutingTest.testChangingPermissions |
5da89ce
to
e51a456
Compare
@@ -73,7 +73,7 @@ public function __construct() { | |||
$this->res = \CRM_Core_Resources::singleton(); | |||
$this->angular = \Civi::service('angular'); | |||
$this->region = \CRM_Utils_Request::retrieve('snippet', 'String') ? 'ajax-snippet' : 'html-header'; | |||
$this->pageName = $_GET['q'] ?? NULL; | |||
$this->pageName = \CRM_Utils_System::currentPath(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Joomla does not use $_GET['q']
which is why we have a function for this.
e51a456
to
86dc644
Compare
@totten I think this is done and should be merged now as I've addressed your feedback. However, after merging this I think we should have a dev meeting about afforms and url params and the potential pitfalls of having more than one afform on the screen. |
Afforms are meant to work as standalone directives so routing on the standalone page was meaningless
86dc644
to
0a7837c
Compare
Cool, I did a new Agree on both comments. On having multiple subforms that consume params - yeah, that can be messed up. It's not strictly problematic like with routing -- you could design the page params to either match-up purposefully or to avoid naming conflicts. It's conditionally problematic - if there's an accidental naming conflict. We've been following a pattern to date where a form opts-in to different media (eg I have been wondering how we should present the toggle for WP shortcodes, eg
Might be a good time to settle that since we have two output media developing (mail-tokens #19660 and WP shortcodes). |
Overview
This removes an unused dependency from the Afform standalone page, making it more flexible and reusable.
Before
Afforms in "standalone" mode required ngRoute, limiting the usefulness of the Afform Standalone controller as it could only handle one afform per page.
After
Dependency removed.
Technical Details
Might need to do more to get AfformStandaloneCtrl working in multiple instances per page because
ngApp
is also limited to single-use. But this is a start.