- Issue created by @lauriii
- š§šŖBelgium Wim Leers Ghent š§šŖšŖšŗ
This component already exists, for the very reasons cited here:
tests/modules/sdc_test_all_props/components/all-props/all-props.component.yml
.It's existed for
\Drupal\Tests\experience_builder\Kernel\SdcPropToFieldTypePropTest
, precisely for ā but long prior to us having the ability to generate widgets, just finding viable storage candidates.(It has even existed before the
0.x
branch was created!)And I'm literally on the path towards making exactly the intent of this issue feasible: #3461499-16: Introduce new `PropShapeInput` config entity type ā .
- Status changed to Postponed: needs info
about 1 month ago 9:41am 25 July 2024 - š§šŖBelgium Wim Leers Ghent š§šŖšŖšŗ
So, can you please check if anything is missing from that
all-props
component? If you can't find anything missing, we can mark this issue as obsolete and unpostpone the issue it is blocking. - Assigned to bnjmnm
- š«š®Finland lauriii Finland
Great, thank you for pointing that out!
@bnjmnm can you confirm if this meets the requirements or if more work is needed?
- Status changed to Needs review
about 1 month ago 10:41am 25 July 2024 - Issue was unassigned.
- Status changed to Active
about 1 month ago 11:38am 25 July 2024 - šŗšøUnited States bnjmnm Ann Arbor, MI
If this winds up providing all the necessary fields, then there should be at least an install hook added to
sdc_test_all_props
so it is available as a Page Builder component as soon as the module is installed.I attempted to add the "All Props" SDC as a Page Builder component and got
ValueError: "" is not a valid backing value for enum Drupal\experience_builder\SdcPropJsonSchemaType in Drupal\experience_builder\SdcPropJsonSchemaType::from()
If the error š is addressed then I can review how this renders in the edit form and recommend if anything additional is needed.
(I also noticed
sdc_test_all_props.info.yml
has a dependency onsdc:sdc
which shouldn't be needed now that it is in core) - Assigned to bnjmnm
- šŗšøUnited States bnjmnm Ann Arbor, MI
Addressed
"" is not a valid backing value
and digging into additional errors that surfaced once that was addressed. - šŗšøUnited States bnjmnm Ann Arbor, MI
I'm getting an error on any props of
type: integer
InvalidArgumentException: Field test-integer is unknown. in Drupal\Core\Entity\ContentEntityBase->getTranslatedField() (line 616 of /Users/ben.mullins/Sites/drupal/core/lib/Drupal/Core/Entity/ContentEntityBase.php).
Which happens after calling
getWidget()
informTemporaryRemoveThisExclamationExclamationExclamation
- Assigned to Wim Leers
- š§šŖBelgium Wim Leers Ghent š§šŖšŖšŗ
I'll test this with š Support complex SDC prop shapes: introduce (Storable)PropShape to compute field type storage settings Fixed ā that issue should make this one much easier to push forward š¤
- Issue was unassigned.
- Status changed to Postponed
about 1 month ago 2:31pm 25 July 2024 - š§šŖBelgium Wim Leers Ghent š§šŖšŖšŗ
Working on it, and as of commit e83470a84c89907bcdd73898485e5a78f03a383f, you should be unblocked here, @bnjmnm. For now, it'll require you to have š Support complex SDC prop shapes: introduce (Storable)PropShape to compute field type storage settings Fixed applied.
But really, the point here is that such an "all possible shapes" component exists (which it already does) and has a working right sidebar (which will be the case after š Support complex SDC prop shapes: introduce (Storable)PropShape to compute field type storage settings Fixed ).
So: there's nothing left to be done here for now, except wait until #3461499 is done, and then revisit.
- š§šŖBelgium Wim Leers Ghent š§šŖšŖšŗ
I've been working on expanding the test coverage over at #3461499 to verify that we truly can generate all the necessary widgets for the
all-props
component. I've already found a number of edge cases that indicate why @bnjmnm could not possibly have gotten this to work šThis is definitely hard-blocked on #3461499, and soft-blocked on #3463999 ā we won't quite need that to be finished too, because I can just attach a sample config entity here š
Stay tuned.
- Assigned to Wim Leers
- Status changed to Active
about 1 month ago 8:45am 1 August 2024 - š§šŖBelgium Wim Leers Ghent š§šŖšŖšŗ
- š§šŖBelgium Wim Leers Ghent š§šŖšŖšŗ
LOLOL an hour worth of debugging only to find out that SDC prop names may contain dashes but Twig variable names must not: https://github.com/twigphp/Twig/issues/1967 ā for example
{{ test-REQUIRED-string }}
is compiled by Twig to:
// line 5 yield $this->extensions['Drupal\Core\Template\TwigExtension']->escapeFilter($this->env, ((($context["test"] ?? null) - ($context["REQUIRED"] ?? null)) - ($context["string"] ?? null)), "html", null, true);
ā¦ which means that the DX of SDC is leaving a lot to be desired: if Twig doesn't support this, and SDC supports only Twig, then I expect SDC to not allow this either. This is so utterly absurd š¤Ŗ
Tagging to file an upstream SDC bug report.
- Merge request !134#3463583: Use the `all-props` component for testing form backend + frontend integration ā (Merged) created by Wim Leers
- Assigned to bnjmnm
- Status changed to Needs work
about 1 month ago 1:25pm 1 August 2024 - š§šŖBelgium Wim Leers Ghent š§šŖšŖšŗ
Voila, @bnjmnm is now unblocked š
- Status changed to Needs review
about 1 month ago 2:11pm 1 August 2024 - š§šŖBelgium Wim Leers Ghent š§šŖšŖšŗ
@bnjmnm Feel free to merge when you like ā this is ready as far as I'm concerned.
If you want, you can add more props to test against. But if this suffices, then I think it's ready for you to confirm that you can trigger the above (by installing
sdc_test_all_props
), then you and others working on the front end can test/improve field widgets using this SDC š - Issue was unassigned.
- Status changed to Needs work
about 1 month ago 4:59pm 1 August 2024 - šŗšøUnited States bnjmnm Ann Arbor, MI
I can't add this as a Page Component, but I suspect it will be easy to address as the error specifies a line that was committed only yesterday
Warning: Undefined array key "#markup" in Drupal\experience_builder\Controller\SdcController->preview() (line 370 of /Users/ben.mullins/Sites/drupal/modules/custom/experience_builder/src/Controller/SdcController.php) #0 /Users/ben.mullins/Sites/drupal/core/includes/bootstrap.inc(108):
- Assigned to Wim Leers
- š§šŖBelgium Wim Leers Ghent š§šŖšŖšŗ
Odd. Will double-check tomorrow with a fresh install.
- Assigned to bnjmnm
- Status changed to Needs review
about 1 month ago 8:31am 2 August 2024 - š§šŖBelgium Wim Leers Ghent š§šŖšŖšŗ
composer require drush/drush vendor/bin/drush si standard --account-name=root --account-pass=root --site-name=LiveDemoWhatCouldGoWrong --yes vendor/bin/drush pm:install experience_builder --yes vendor/bin/drush pm:install sdc_test_all_props --yes
Followed by creating node 1, then going to
/xb
, results in the screenshot I posted in #17. I didn't need to change anything.Any chance you just checked out this branch and already had the
sdc_test_all_props
installed, which would've caused the optional config to not get installed, which would explain ? š¤(Fixed the warning in #19 though!)
- Issue was unassigned.
- šŗšøUnited States bnjmnm Ann Arbor, MI
Starting from scratch gets the component to install and I can see it in the XB UI and it does its job nicely
The problem I ran into earlier (or something similar to it) seems to be happening still - the edit/add form for this SDC isn't working. This is less urgent since it installs with the module now, but here's what I'm running into when I try to edit in
admin/structure/component/edit/sdc_test_all_props
:If I try to edit the all-props component when I click submit on the form, the submission is actually blocked by JS and there are console errors:
An invalid form control with name='sdc_test_all_props+all-props[<<daterange field|UUID field>>][field_type]' is not focusable. ā
. It was reasonably easy to conclude this is due to the date range and UUID props not having field types selected - but still required more sleuthing than I'd expect a normal user to do (I could see this being out of scope here since it is a test-only component)Setting the field types in the form allowed me to submit the form, but I got the error
Error: Call to a member function massageFormValuesTemporaryRemoveThisExclamationExclamationExclamation() on null in Drupal\experience_builder\Form\ComponentEditForm->copyFormValuesToEntity() (line 317 of /Users/ben.mullins/Sites/drupal/modules/custom/experience_builder/src/Form/ComponentEditForm.php).
Since this is test only and VERY helpful to have, I think it's reasonable to address the edit form issues in another issue. For the time being, I recommend adding something specific to the edit form for this component that warns about this and perhaps makes the edit fields unavailable "This test-only component is provided as-is and cannot be edited" so we don't run into contributors losing their (or our) time assuming this would work.
- Assigned to bnjmnm
- š§šŖBelgium Wim Leers Ghent š§šŖšŖšŗ
the edit/add form for this SDC isn't working.
+
[ā¦] when I click submit on the form, the submission is actually blocked by JS and there are console errors [ā¦]
Ahhh, now I get why you said it wasn't working š
That's known. Because configuring each component's field type + widget etc in detail using a UI is non-desirable per @lauriii. That's why š Support complex SDC prop shapes: introduce (Storable)PropShape to compute field type storage settings Fixed ignored that limitation, and why š Auto-create/update Component config entities for all discovered SDCs that meet XB's minimum criteria Fixed is next, and why once that is done, š [PP-2] Remove Component config entity's add/edit form Postponed will happen.
TL;DR: the current
ComponentEditForm
only works for components whose props are simple enough, not for arbitrarily complex SDC props.I think it's reasonable to address the edit form issues in another issue
Ahead of you ā it will disappear shortly, in š [PP-2] Remove Component config entity's add/edit form Postponed š
For the time being, I recommend adding something specific to the edit form for this component that warns about this and perhaps makes the edit fields unavailable "This test-only component is provided as-is and cannot be edited" so we don't run into contributors losing their (or our) time assuming this would work.
Good point, we should make sure people don't waste time until #3464025 lands, done:
ā¦ and just to make sure: other components do not get this warning:
-
bnjmnm ā
committed 117913d3 on 0.x authored by
Wim Leers ā
Issue #3463583 by Wim Leers, bnjmnm: Use the `all-props` component for...
-
bnjmnm ā
committed 117913d3 on 0.x authored by
Wim Leers ā
- Issue was unassigned.
- Status changed to Fixed
about 1 month ago 7:17pm 8 August 2024 - šŗšøUnited States bnjmnm Ann Arbor, MI
I tried this out and it works great. If anything is missing NBD - we just add to it later - as-is this unlocks a bunch of things we've been aiming to do.
- š§šŖBelgium Wim Leers Ghent š§šŖšŖšŗ
#25++
This makes doing š Component props form: map textarea, bool, and select elements to React components Fixed simpler, see #3462310-14: Component props form: make form elements match design ā ā and in fact, with this now in, a superset of the 5 things listed in #3462310 is now readily available š
See y'all in š Component props form: map textarea, bool, and select elements to React components Fixed !
Automatically closed - issue fixed for 2 weeks with no activity.