Assigning back to myself to make requested changes.
I've created an MR for adding weight to Segments, I haven't addressed anything in the Drupal UI for weighting as I saw this issue: 📌 Revisit segments UI: do we need a Drupal UI? Active and I assume it may get addressed there?
I've also not addressed discussion in #2/#3 as seems thats not currently confirmed. But we might need 📌 Provide a locked Default segment Active to land first? Or it get handled in that ticket after this lands?
thoward216 → made their first commit to this issue’s fork.
thoward216 → changed the visibility of the branch 3541501-update-phpcs-config to hidden.
thoward216 → created an issue.
thoward216 → made their first commit to this issue’s fork.
Moving to needs review to get some eyes on the one bit of feedback that I've attempted to implement and comment added on the MR, may need further work. All other feedback should be addressed.
I believe this would be happening because when you first create a new code component it will be using the name input by the user to generate the machine name of the component. Upon renaming the component, it already has a machine name and that is something that wouldn't change once created, so it's just updating the label at that point.
I've updated some tests that were failing, updated the access check to account for the new route and added to the test.
thoward216 → made their first commit to this issue’s fork.
After rebasing and uncommenting the schema changes as mentioned in #13, there are a number of failing tests where the validation messages are not as expected.
An example: 'versioned_properties' => 'This value should satisfy at least one of the following constraints: [1] This value should be of the correct primitive type. [2] This value should be of the correct primitive type.'
To me this doesn't look right and the message is also duplicated. I spent some time investigating a bit further with @f.mazeikis we're not sure that the constraint AtLeastOneOf can be used with sequences? Looking at the core test AtLeastOneOfConstraintValidatorTest it only ever tests with strings or integers.
thoward216 → changed the visibility of the branch 3538306-update-phpcs-config to hidden.
thoward216 → created an issue.
thoward216 → made their first commit to this issue’s fork.
Coming back around to this, as per #10.
A new components config with one version looks like this:
uuid: d973b1d2-df01-4f89-a309-fe83f34c9d9a
langcode: en
status: true
dependencies:
config:
- experience_builder.js_component.test
active_version: 8fe3be948e0194e1
versioned_properties:
active:
settings:
prop_field_definitions: { }
fallback_metadata:
slot_definitions: { }
label: test
id: js.test
provider: null
source: js
source_local_id: test
category: '@todo'
New versions created adds them under `versioned_properties` as expected but as you can see above the hash `8fe3be948e0194e1` is not in `versioned _properties`. So either the above isn't structured as expected (though I'm sure there are a number of tests around this) OR the original constraint that was commented to add later maybe stale?
Started having a brief look into this but I can't seem to reproduce. If I create two patterns with the same name the second one then gets an id as per the code in #2.
Thanks @larowlan - I've manually tested this and everything is working as expected and the output of the component tree keys is what I was expecting to see also. I've just opened an MR to run the pipeline.
I've started some investigation into this and it doesn't look to be the same issue as #3534971 and looks to be due to #3526127 and the way that the function generateComponentTreeKeys() builds the keys. The array that is passed into the generateComponentTreeKeys() function contains all 10 items in the tree in my case, but when I go through the function I only end up with 8 items.
I did a test by commenting out the set() function in Pattern entity and everything saves as expected so that looks to prove where the root cause looks to be. Will continue to debug this further.
Moving back to needs work as rebased with latest 0.x and resolved conflicts but there are now some failing tests.
Updating issue summary.
thoward216 → made their first commit to this issue’s fork.
I think this is failing as 'versioned_properties' does not contain the hash of the 'active_version' as its simply keyed as 'active'. So I'm not sure this is the correct constraint to be added here?
Had a look into this issue and got the library attached using similar code as shown in #2 but the preview still did not appear to update as expected. Requires further investigation.
Have updated the the MR and things are looking better. Now getting errors around the validation for the version constraint that was added/uncommented.
https://git.drupalcode.org/project/experience_builder/-/jobs/5827036#L4725
XB now requires 11.2 so I believe this can be actioned.
There we also some changes in Translation test where the 'article' tag selector needed to be removed. So these may also need to be reverted/reviewed.
See: https://git.drupalcode.org/project/experience_builder/-/merge_requests/4...
thoward216 → created an issue.
thoward216 → made their first commit to this issue’s fork.
Looks like theres now a number of config schema issues now occurring: https://git.drupalcode.org/project/experience_builder/-/jobs/5657671#L2402
These looked to be "ignored" before here
Not sure if these should be resolved in this MR.
Tests are now all passing on this.
Oh yes! That was a silly mistake, have fixed that now!
I've fixed those two PHPUnit test failures, commented out the failing e2e test for now and also added a bit to the components.md - let me know if this needs expanding.
thoward216 → created an issue.
I am able to reproduce this issue with SDCs, although I am also seeing an issue whereby if I toggle a boolean field from true to false, save the changes and then refresh the page the toggle has gone back to the true state. Looking at the data saved in the database it has saved correctly there so this looks to be a UI bug.
This can easily be seen with something like the Shoe Badge SDC and toggling the "pulse" boolean and saving the pulse effect in the preview does not happen but the toggle is showing as true.
I wonder if this is some of the root cause to the default values not working as expected here? Or could be something separate, if so I can create a new ticket for this.
As mentioned in #2 🐛 Setting default values does work for boolean props Active it does look like the default value used is the first value in the examples.
thoward216 → made their first commit to this issue’s fork.
Understood, thanks for context here @berdir. It will likely need further investigation by the Site Studio team.
thoward216 → created an issue.
All the tests look good. Assigning to myself to look at adding a test for this next.
After merging the first one there was a conflict in the remaining MR, I have rebased that one again.
Both MRs have been rebased.
Moving back to needs work as theres a PHP unit test failing.
I've found the code that looks to have been causing this, it was recently changed. I've created an MR to run all the tests and will need further testing, its likely it'll need more logic rather than just removing the enable function.
Both MRs are now passing tests and ready for review.
I'm able to reproduce the same behaviour as @jatingupta40 - disabling an SDC component shows that component in both lists. Interestingly under "disabled components" the status for the component shows as "Incompatible" but the reason shows as "Manually disabled" I think that the status here maybe at least part of the problem and it should be marked as "Disabled" and not "Incompatible".
Hi @libbna, I think I can help here.
I would first take a look at the pipeline for the MR and you'll see a number of PHPCS issues, which is due to the reformatting @wimleers mentioned.
In terms of the UI, it is currently still expecting `source_code_js` to be valid, if you search the code base for `source_code_js` you'll see it is referenced in a number of the UI React files, these will also need updating.
Hopefully that makes sense and you can proceed.
@tedbow are you still actively working on the failing phpunit tests for 3473761-empty-slot MR?
Tests are now all passing with the update approach. Moving to needs review.
@wimleers thanks, yes I'm unblocked on this.
@wimleers - thanks for the context and suggested path forward in #22 🐛 SDC prop of `type: string` with empty string listed in its `enum` results in broken input UX Active will take a look into this.
@tirupati_singh - I've pulled in the latest commit locally and tested your above component and it appears to work as expected, it does not show in the component library in experience builder and appears within the "disabled" section under "/admin/appearance/component/status" with the reasons - see screenshot.
I've updated the approach here to match the proposed solution, added a new test SDC and updated tests as needed.
After updating the approach, a number of tests are now failing due to them using the SDC sdc_test:my-cta as this is now not compatible with XB. (as it contains an enum with an empty.) It appears it is used in a lot of other tests which now error, due to the tests trying to use it. I'm not clear on the path forward for this yet.
thoward216 → changed the visibility of the branch 3503087-sdc-prop-of to active.
thoward216 → changed the visibility of the branch 3503087-sdc-prop-of to hidden.
Moving back to "needs work" and assigning myself to look into the failing tests.