Add opt-in for controlling what form elements are rendered via civictheme system #1306
Conversation
richardgaunt
added
State: DO NOT MERGE
| @@ -100,7 +100,7 @@ function _civictheme_form_alter__non_form_elements__link(array &$element): void | |||
| * | |||
| * @SuppressWarnings(PHPMD.StaticAccess) | |||
| */ | |||
| function _civictheme_form_preprocess_hidden_form_elements(array $variables): void { | |||
| function _civictheme_form_preprocess_hidden_form_elements(array &$variables): void { | |||
There was a problem hiding this comment.
This is needed, non exposed filters were not being added.
form_hidden_fields
these be used when passed as query parameters.
There was a problem hiding this comment.
ok great thanks for checking
There was a problem hiding this comment.
So we should be deleting this function then.
|
this is undoing the work that was done to make Drupal look 100% as per design system and be a true design system 😢 we had similar approach in 1.0.0 and the form elements were all over the place without any systematic control. instead of creating an opt-in system, CivicTheme should implement support for those modules and forms where the form elements look strange. |
@AlexSkrypnyk you make an excellent comment thank you for looking into this. This is my current viewpoint: By stating directly what elements CivicTheme can render correctly we are ensuring it renders those supported elements as per the Drupal design system. For form elements that we do not know whether they work with CivicTheme we allow Drupal to takeover the render system. To me this seems like the most sensible approach. Supporting every form element we want to in CivicTheme is a Sisyphean task, it's not desirable for CivicTheme to have civictheme implementations for 10s of form element types, the maintenance workload of keeping these form elements working is well beyond the ability of this project. There are custom attributes, custom JS libraries, contrib module updates and fixes, core changes that would need monitoring, adapting and maintaining. On top of that there are CivicTheme releases that every developer out there would need to keep on top of to ensure all those fixes are carried forward to their themes as those module's update. So we are asking for a lot of developer resources not just from within CivicTheme but from every project relying on CivicTheme. By being explicit by what is directly supported and then defaulting back to Drupal core's rendering pipeline, we get to keep the great changes you have made to uikit form elements and use them when we know they are going to work and throwback up to Drupal to handle when we dont know. By doing this I've seen that it does it quite nicely for example, the Doing it the way suggested, I would need to directly support password_confirm, create a form element, create a form preprocess to map the password_confirm attributes to the custom form element, test and ensure the JS works correctly. Then every release and drupal update we need to test this custom implementation to ensure it still works as expected. But with this approach, we do not support I get a working password confirm form element out of the box through this system. Many of the advanced form elements utilise more base form elements and so if we throw to the contrib module to provide the advanced form element template, it then utilises civictheme for the inner form elements. This PR is still a work in progress and need to examine the effects of this change on webform. |
|
@richardgaunt And I understand where you’re coming from. But this is not about just making it work with Drupal. This is about breaking the concept of design system. We could, as well, just render some other elements using basic styles “because CivicTheme doesn’t support it”. The whole idea is to create a design system with as many elements as possible to cover many cases. Other ui kits do it, but not CT?! This also sets the precedent of adding work arounds instead of addressing the problem - CT needs more components if it wants to be a world-class design system. |
Thanks @AlexSkrypnyk I also appreciate where you are coming from and I appreciate you taking the time to write this feedback. And I agree "this is not just about making it work with Drupal" which is why I would argue we are not attempting to support every form element in Drupal at some point there are elements we do not want to support as they are not in line with CivicTheme so even if we attempt to support all the form elements we do not want to which is why we opt-in, so 3rd party modules can provide the missing support. This PR is not being rushed in without community feedback once complete I will be raising in DO to attempt to get feedback. The feedback I have received however from a series of community members is they want CT to integrate smoothly with Drupal. |
|
@richardgaunt maybe you could please add some information about the purpose of this pr to a description so that others in the community would know the context |
| @@ -70,9 +70,12 @@ function _civictheme_form_alter__non_form_elements(array &$elements): void { | |||
| case 'link': | |||
| _civictheme_form_alter__non_form_elements__link($elements[$element_key]); | |||
| break; | |||
|
|
|||
| case 'html_tag': | |||
There was a problem hiding this comment.
Adding markup handling in this function
| * @ingroup themeable | ||
| */ | ||
| #} | ||
| {% include '@molecules/basic-content/basic-content.twig' %} |
There was a problem hiding this comment.
@richardgaunt This breaks the admin menu when logged in
Checklist before requesting a review
Issue #123456 by drupal_org_username: Issue titleChangedsection about WHY something was done if this was not a normal implementationChanged
Screenshots