MR has it working, still need to add tests.
@liquidcms A true revert would either be a patch or overriding the two JS files updated but the quickest way to go back to the earlier behavior is probably to disable the scroll lock immediately after it is activated.
in a custom module add a jQuery event listener on 'dialog:aftercreate', which is triggered immediately after the scroll lock is added.
In the callback, run the same code that is run when the modal dialog closes
const $scroll = $element.find('.scroll');
if ($scroll.length) {
bodyScrollLock.unlock($scroll.get(0));
}
This might not be it line-for-line, I didn't actually try it, but this approach should work - fortunately an event is dispatched right after the scroll lock so it's quite available for disabling.
👏
The followup MR was straightforward and welcome, added.
Went ahead and addressed the feedback from me & @longwave
Ideally someone else should at least look at the bits I changed.
This is what it looks like:
And I'll set to NR once the tests that pass locally can pass on CI 👹
I'm running into the same test failure locally , which is an existing test that also deals with the multi-value textfield widget. It failing on the portion of the test where the items in the multivalue field are reordered. The dragging of the lower item to the top no longer seems to register.
This still works manually, though, so hopefully it's just a matter of tweaking widget-multivalue.cy.js a bit. Unassigning myself for now.
Recategorizing - this was not a bug (i.e. something purported to work that doesn't) but the next step of a work in progress. Either way, this made it apparent that this was something that required higher prioritization.
If everything is ultimately based on the target id, then #17 is fine.
If not, then it would run the risk of missing images on case sensitive file systems. I'd need to debug around to say one way or the other, but perhaps someone else already knows whether or not this is the case.
It looks like there are changes in useCopyPasteComponents.ts that seem adajcent but not directly related to the reported issue. Could a comment or two be added in the MR explaining what the refactoring there is for?
The MR looks good, but could this be expanded to include a checkbox with a default value unchecked, in addition to the existing one with ['value' => 1],?
#41 provided screenshots of the status quo - there are * without the additional explanations requested in the issue summary.
Unless specified otherwise Needs screenshots is asking for screenshots of the implemented fix.
#14.1
Addressed in the MR.
#14.2-3
Will be addressed in
📌
Address CKEditor5 "more items" bar contents not being fully visible
Postponed
#14.4
Will be addressed in
📌
Dialogs opened by CKEditor5 need to be rendered with admin theme
Postponed
Assigning to @larowlan as this is familiar territory for him, but @wim.leers also offered to look into this if that isn't feasible.
If this is the case, why don't we make our client automatically convert every pattern to a regex, which then would allow to use the /…/i syntax PHP supports?
I don't think this would mitigate this particular situation. The issue is that Drupal Core allows adding media that has caps characters in file extensions. Uploading new media items like some-pic.JPG is not performed anywhere that ajv even runs.
We could modify the AJV logic so it sees caps-in-extensions as invalid and thus not making the back-end-breaking PATCH request, but it doesn't seem ideal to not support caps-in-extensions media at all in XB when it works everywhere else in Drupal. I could also see people being frustrated that such media is available to select but they're unaware it's not valid until choosing it and seeing a validation message in the input.
TY
This commit introduced ~12 files of unrelated changes. Since the actual fix is likely a single-file change, it may be worth starting a new MR vs trying to untangle all the unrelated changes.
Looks good!
Good test, the dayjs addition is AOK too.
With the current implementation, the dropdown options include the label names in addition to the expected number/month/etc values and selecting these results in an error in the preview. (notice I was able to choose "Hour" as an option for the Hour dropdown).
This should get addressed and ideally the test expanded to check at least one of the dropdowns to ensure it doesn't regress to again offering the labels as selectable options.
This scope couldn't include component instance forms as there aren't props that support them yet.
The case insensitivity should be specific to the file extensions, not the entire uri
@larowlan is probably the best suited to approve this as it's fixing functionality he introduced successfully that I later broke (see this issue summary), so with that approval I'll merge this in.
As indirectly surfaced by 🐛 Img prop constraints require extension to be lower case Active , doing this will have an additional hurdle of having to implement a way to filter the available items in Media Library UI dialog to the dimensions required by the prop, otherwise we'll run into scenarios where we can choose an item that doesn't meet the requirements, and that selection is accepted by the form due to no additional FE validation, then causes an error when the preview render checks value against the validation constraints.
This is by no means impossible, but it'll be a bit more complex than most prop constraints due to the participation of the Media Library UI dialog
I can't say with 100% certainty this is the same as 🐛 "Publish all changes" gets stuck in a loop sometimes Active , but the chances are quite high. Comment 2 has what are effectively the same steps to reproduce, but reported symptoms are slightly different.
I'm setting this to NR without tests
- because the MR is simple
- it fixes what I was running into
- it directly addresses a change in 📌 Multiple fields widget should fully work in the contexual form Active , which was the commit that git bisect confirmed was the cause
- My Cypress local tests are failing as if they were running on an old Blackberry, making it difficult to get to the point where I would be able to add a few assertions that confirm we don't get excessive requests after changing a media item. This is blocking other work and we're looking into other test approaches so I'd like to see the fix get in if possible.
MR has a fix for this. It should be possible to test this by counting the number of layout update requests after updating a media item, but the e2e best suited for this is failing locally very early in the process so I'm not currently able to evaluate such a test until that gets figured out
In the ~5 years that have passed since I first created the Field Layout contrib version → , I have less overall bandwidth to be an adequate maintainer, so I'm happy to add some additional ones.
Approved but opened 📌 [PP-1] Region name tag visibility when moving components within Active as a followup after discussing with @jessebaker
bnjmnm → made their first commit to this issue’s fork.
larowlan → credited bnjmnm → .
@hooroomoo has already codeowner-approved in the domains I would be able to approve.
tbh it's too darn huge to be able to give it a truly in depth review, but I'm not seeing any red flags and it's really nice overall. I think the best improvements will occur to us once it's actually in the codebase. We still need src and Kernel test signoff, though.
Just so it's official, based on the discussion I feel that my findings in #8 are very much out of scope and just something I happened to discover while reviewing this.
Everything in the MR is adding xb- to the beginning of attributes, so there are attributes that begin with xb-data-. That is not what is being requested. Reiterating the issue summary might cause additional confusion, so just review the summary again and follow it as stated.
This will be fixed by the changes in 📌 Get Options as buttons in Page Data form working Active
Unblocked, as we have landed 🐛 ComponentTreeHydrated prevents serialization Active
I enabled all the Olivero regions and the drop zone sizes got kinda weird. This isn't a fresh install so it's possible this is a hiccup that might not be reproducible but I wanted to capture it now just in case. It's a "cutting off" of sorts but different from 🐛 Preview gets cut off sometimes on a layout change Active
The content drop region got narrow after enabling the regions
I added outlines to each region to see how they are laid out, too.
Good time to de-noise 😎
I took care of some 0.x conflicts since I knew what it would be conflicting with, and also few bits of feedback while I happened to be in there, but didn't want @larowlan to think I was still actively working through all the items and potentially conflicting with work he's doing.
Given the required approvals are made, this passes tests, and there's several other issues that will benefit from the hyperscriptify and test improvements, I'm going to merge this in.
The currently skipped-on-ci e2e tests are also passing
Multiple people can't reproduce. Answer the question in #15 if you'd like this reopened and worked on
This fixes the problem twice-reported in the following issues
- 🐛 "There are no users matching '' error message appears after reloading the editor page Active
- 📌 Get Options as buttons in Page Data form working Active
I started the branch so I can't provide any codeowner approvals, despite this largely turning into something authored by @larowlan
Otherwise, I'd be the approver for Redux-integrated field widgets so if @larowlan wants to be my proxy for that I think that is fine.
Knew the tabledrag targeting in ajax.hyperscriptify.js wasn't great - adding a few attributes per @larowlan suggestion does a much better job of it.
The not-yet-addressed part of this is addressed by 📌 Get Options as buttons in Page Data form working Active , so that should be prioritized
@tedbow ran into what is likely this issue and narrowed it down to the changes made in 📌 Media Library dialogs triggered from page data do not have buttons yet Active
The work done so far in the MR is probably worth keeping, even though they were not related to #3506467 (autocomplete should be adding value, not label, and details contents should be in the DOM).
The parts still not working are probabl related to the #3506467 changes so now we have a place to target our efforts
The problem reported in #67 may already be represented by 🐛 "There are no users matching '' error message appears after reloading the editor page Active where some work has taken place already
I think this is a duplicate of 🐛 "There are no users matching '' error message appears after reloading the editor page Active but just unsure enough that I'm not closing this as duplicate. There's some changes there that are worthwhile, but I'm not sure any of them touch on the specific things caused by 📌 Media Library dialogs triggered from page data do not have buttons yet Active
I noticed there is activity from several people on this issue after a fix was committed and the issue closed.
If anyone above are hoping their comments result in additional changes to Drupal core, the chances of that happening are infinitely higher if it's reported in a new issue.
It is very rare that an issue is truly "reopened", and in those rare instances it's pretty much always within a few days of it being closed.
bnjmnm → changed the visibility of the branch 3484395-support-props-that to hidden.
bnjmnm → changed the visibility of the branch 3484395-wysiwyg-ckeditor5-widget to hidden.
Not much point working on this until we can get CKEditor 5 loading on textareas 📌 CKeditor not loading on formatted text fields in the content entity + component instance forms Active so postponing on that
The current MR is outdated to the point that it's probably not worth continuing, and will likely be less relevant after #3512867 so I'm going to hide that one and recommend a new MR starts once we have CKEditor 5 working in the fields that are configured to use it. #3512867 already has it the editor properly loading with formatted text fields in the Page Data form
I expected way more of the fields added in xb_test_article_fields to either "just work", or not work in a manner that didn't adversely impact the form if they weren't fields that had value changes.
In the case of language, options-as-buttons (radios), and possibly others, it's clear this isn't the case - they are enough to deserve their own issues IMO. In particular, if the problems impact the ability to test any fields in the form, a separate issue might be good. This issue could then be specific to adding tests for fields that either work already, or are broken in a way that doesn't contaminate the entire form.
It looks like the branch here and the MR in 📌 Get Options as buttons in Page Data form working Active are tackling the options-as-buttons issue in different places and I suspect they might be more complementary than conflicting. My preference is to see the work on option buttons here brought over to that issue. BTW I describe a wall I hit in comment 4 → but perhaps the approach here gets around that.
MR has a few bits that are necessary to get this working, but nothing touching on comment #2 yet. Some way of calling EntityAutocomplete::valueCallback or doing the equivalent will likely be needed.
- On initial load, the node author field has a value formatted like this:
username (USER_ID) - After the first autosave, the field value becomes just the user id.
- Now, when the form reloads (such as a refresh that uses autosave data) there is just a 2 in the field and when the validators are called as part of the build,
\Drupal\Core\Entity\Element\EntityAutocomplete::validateEntityAutocompletewill call$match = static::matchEntityByTitle($handler, $input, $element, $form_state, !$autocreate);and there is no title to match, because$inputis just the user ID, not the name.
I suspect this will be addressed if we address the process that converts the field value from username (USER_ID) to just the id.
The initial POST to https://drupal.test/xb/api/layout/node/{nid} fails due to the radio option being an array instead of the expected string
The option is being sent as a string on the FE
The last XB line of code before the error occurs is in \Drupal\experience_builder\ClientDataToEntityConverter::setEntityFields
$form = $this->formBuilder->buildForm($form_object, $form_state);
And during that build it eventually culminates in The submitted value type array in the XB Options (Buttons) element is not allowed. and a few other type validation issues. The $entity_form_fields sent to the build process has many similarly structured fields -> values so at first glance there doesn't appear to be anything wrong with the structure itself, so I suspect it's an issue with setting the correct validation expectations... but that is just speculation ATM. Will explore further.
This isn't a bug - it's a feature request.
The issue summary is still inaccurate. It should be presented as a desire to have a different icon present depending on open/closed state.
@julio_retkwa appreciate the attempt but the changes to the summary are not at all what was reported, the steps to reproduce and proposed solution seem like they are from an entirely different issue that happens to be media library related. Among other things you added "Added a js behaviour to scroll up on field media modal open." as the Proposed Solution despite not touching any JS in your MR or the issue having anything to do with scrolling.
I updated the IS to address this.
This also needed a manual run of the tests currently skipped for headless: prop-types.cy.js and autocomplete-props.cy.js:
This seems fine - AFAIK the only motivations behind not doing this initially is no longer relevant at this stage in the project. After merging 0.x with this I did encounter some problems that I addressed largely in XBTemplateRenderer, and also extended one of the e2e timeouts so it spanned the wait of the autosave interval + a little additional time.
Needs ui codeowner approval.
Got this working by querying from the available ref
const hiddenSelect =
selectRef.current.parentElement?.querySelector<HTMLSelectElement>(
'select[aria-hidden]',
);Doesn't feel the most solid to me, but maybe it's OK. So far seems to work.
My next step was to see if changing editors worked, and ran into a situation that will likely require nontrivial refactoring. The underlying cause may cause problems for more than just this use case.
Radix <Select> does result in a <select> tag in the UI. It is instead a button that triggers the appearance of selectable options.
Drupal.behaviors.editor very much expects a <select>, in some cases not adding the listeners unless that is the element type.
if (editor.tagName === 'SELECT') {
$this.on('change.editorAttach', { field }, onTextFormatChange);
}
The Radix select does have a hidden <select>, which is not exposed as a ref in @radix-ui/themes, but looks like it miiight be accessible if we access it more directly instead of the theme. I did some exploring and this doesn't look like something that can be done quickly, but there is a forwarded ref in the Select primitive, so it should be possible somehow. However, even if the <select> can be accessed via ref, we may still run into a limitation with some part of Drupal due to the select in the UI not being an actual <select>
Does CKEditor 5 itself also work?
The CKEditor 5 editor works - you can format the text using the various buttons like one might expect.
Do the contents of the edited-by-CKEdtor5 field get saved? That part does not appear to work. It that can be tackled in a separate issue once this lands.
It looks like this is likely related to a Chrome extension. I need to try one by one to see which. It probably means this is less urgent than I initially thought, but some of that might depend on which extension(s).
MR I just opened has logic that gets CKEditor5 to appear. It's not ready for prime-time yet but the underlying issue has been identified and there is a solution.