Between this issue and
📌
Confirm Semi-coupled form elements can work with State API visibility
Active
, if we want to continue using Radix we need to have a way to customize the attributes sent to the input elements within the component. I filed an issue with primitives about doing this with Checkbox, and if that is welcomed I'll do the same in Radix themes for the TextField component
The MR has some tests fail - not sure if it's truly broken or conflicts with aspects of the test setup that would not occur in a real Drupal install.
Re #6 Yep that's what it looks like. cache_discovery - cid library_info:olivero is the culprit. If this cache item is first generated when the XB UI loads, the $route_name === 'experience_builder.experience_builder' condition is true and the library info cache will omit the extra assets. I suspect when that scenario happens then Olivero risks having some assets missing, but I haven't confirmed this.
I updated the steps to reproduce and added a failing e2e test that admittedly does not follow the exact steps as summarized, but still runs into the problem. In the test, it fails due to an error vs a non-passing assertion because the pile of Olivero assets include navigation JS that assumes certain elements are present and it errors when those elements aren't found.
Needs a @traviscarden signoff because of the change to openapi.yml
When I was working on ✨ Add support for the inline variation of form elements Needs work over four years ago, I created this ussue as this was one of several things that might improve the feasibility of that issue landing. TBH I think it's very unlikely that issue will ever land and getting something like this in will not increase its changes. The changes are too far reaching and disruptive - even if core worked solidly there would be countless problems with the contrib.
I'm just going to close this and someone can reopen if there's a really compelling reason beyond nudging forward a Claro issue that is unlikely to ever be completed.
Discussed this with @longwave, and one big next step here besides general review is to create tests that surface the difference cases where there isn't a 1:1 map between field values, prop values, and widget values. @longwave offered to start on that.
Can we make those separate issues that are referenced here? Aside from this being a large review already, there are major changes to the .module file that require quite a bit of manual steps to reroll.
In 📌 Create extendibility proof of concept that also serves as documentation-by-example Active I've added a test module that provides a barebones implementation of the Semi Coupled theme engine . I think we're running into a usefulness plateau with attempting to explain it all in a single document, and routing some of that documentation to the test module might prove more helpful.
I also added 📌 Create readme for hyperscriptify library Active to create a Readme for Hyperscriptify so that explanation lives within the package that provides the functionality. We can certainly link to it etc but I think having it with the package can help demonstrate that the functionality it provides is not reliant on XB or Drupal.
bnjmnm → created an issue.
Added a test module in the MR and found that the autocomplete inputs (which had been proven to work in the past but never got test coverage) no longer work due to the input now using the Radix TextField component. This component intercepts a few props, including className and applies them to the input's parent <div> instead of the <input/> and Drupal's autocomplete initializes based on the presence of .form-autocomplete on an <input/> element.
It's probably not all there yet, but this is where i leave off today. The PSD file is available for anyone to edit a copy, too.
Still working on editing the image from #43 it's a chungus Photoshop file.
This is addressed much more easily with the schemas fully available which is happening in 🐛 Premature prop validation can break the UI Active , so I addressed the reported issue there + included tests to verify it is fixed. Get that one reviewed/codeowner signed and the symptoms reported here are fixed in a less hacky way than what can currently be done off 0.x
The reported bug was addressed in another issue, but the test is worth keeping to ensure this does not regress. Remove the changes to RightClickMenu but keep the test
Feedback addressed
-
I left one small suggestion for rewording things in xb_stark.info.yml
. Good call, done!
-
@bnjmnm, can you please see if you're happy with how I did the form alter in 0332d920? There might be a nicer way to do it.
It's good.
-
On a call last week @jessebaker had the suggestion to use include and refer to the original Twig templates that way in our process_as_regular_twig template overrides. Would we like to do that as part of this issue, or in a follow-up?
Thanks for the reminder! With that change this is an MR that reduces the total LOC by 139!
Just need a few additional codeowner signoffs.
alexpott → credited bnjmnm → .
Gitlab does not list me as a codeowner that can approve this, which I assume is due to me being responsible for some of the earlier commits on the MR, but all the changes since mine are good by me.
Anything I spotted that I'd typically point out in a review happens to be something that either will be refactored out of existence within a week or two, or related to interactions that don't have formal requirements, so it's better to have this committed so it can help those requirements be discussed.
This is kind of a weird one to approve as there's no great way to test it without literally adding support for non SDC but that is what THIS issue is blocking. Worst case this is a little incomplete but does no harm, so lets let it in to help the other issues move forward.
This still needs src signoff - I'm going to assign to @tedbow who I know is around for the next two days, but anyone in that group could take care of this.
A nice bonus is that the Media Library widget is also rendered with xb_stark
Although XB stark defaults to using Semi Coupled and rendering with React, the MR includes Twig templates that ensure Media Library is still rendered with Twig. This was added to address an issue where core/drupal.dialog.ajax needs the completed HTML earlier than Semi coupled can (currently) provide it. If we really want to render the Media Library widget in React I'm sure this can be done, but it might be worth keeping as is since it's the format the Media Library UI is known to work well with, and there's no need for it to communicate with redux.
This may open options up to more easily address 📌 Media Library dialog styling Active if we want XB specific styles instead of using the admin theme.
bnjmnm → made their first commit to this issue’s fork.
Good News I think this image helps make sense of some of the more confusing bits
Bad-ish News (more work.. but better code) Seeing it mapped out like this made it clear there are some additional changes I should probably make to make the DX less confusing.
See MR
Can you please confirm that State API is NOT supposed to be working yet for this form? E.g. “Provide a menu link” checkbox.
Can you please confirm that autocomplete inputs are NOT supposed to be working yet for this form? E.g. “Tags” field.
During early evaluation/proof-of-concept stages we confirmed that these functionalities can work, but this was before we had solid tests in place where we could ensure that functionatly continuesto work. It's very possible that something changed in the past several months that changed what is needed to get those working. I don't think this issue scope needs to be concerned with any functionality we have not confirmed to be working with e2e tests. I created 📌 Confirm Semi-coupled form elements can work with autocomplete Active and 📌 Confirm Semi-coupled form elements can work with State API visibility Active
The A11y test is catching this contrast error where the bg/fg ratio is 3.26:1 when a smaller control like this needs to be at least 4.5:1
Reinstall takes care of it.
The tests now work - not quite ready to set to NR as we should test scenarios like pasting a component that has been deleted and if possible, pasting between tabs.
wim leers → credited bnjmnm → .
Pushed the MR in a very WIP state so I'm switching it to draft- needs some cleaning up that I don't have time for atm, but it now works like a true copy paste. The original can be deleted but still pasted, and pasting can happen from one tab to the next.
It is using localStorage, not clipboard, since clipboard only works with https and it's not uncommon to work on a non-https local. If we really want clipboard support I recommend doing it in a separate issue as we will need to determine if it is worth having two copypaste apis to support, and the possible confusion that might occur from pasting a stringified layout / model object into a document.
bnjmnm → created an issue.
Sorry for the chungus MR, but it is ready.
Responded to my own feedback and got the tests passing
Anything additional come to mind? This is on 0.x with a cleared cache.
Not reproducible on my end, please provide additional steps.
99% done - FieldTypeUninstallValidatorTest is failing and I'm not yet sure why. Unassigning myself to open this up to contributors who have had more exposure to that test and might be able to figure it out more quickly. In the meantime I'll keep debugging to see what is happening.
There might be glaring issues I failed to notice due to documenting-fatigue but I think this is a good point to officially NR it. It's a huge step in the right direction, so perhaps the main goal here could be making sure nothing is flat-out wrong. We can iterate on the quality later (and as the functionality evolves)
I'm not particularly attached to either return type and don't recall why the props form was not a JSON response. It sounds like it's not a huge effort to switch it to a render array, and switching the other response to JsonResponse also seems OK. For this issue, I'd personally recommend whichever switch requires the least amount of effort, and if it happens to be the Render Array approach we can have a followup to look into making them both JsonResponse
I created an MR with a good starting point for someone to pick up - the form is now rendering in the "Page Data" tab, and using React.
There's plenty to do in addition to me getting the form to appear there. There are fields there we don't want, some of the code used to get this working might be repeating logic that could be reused, the current rendering is too wide for the available width, etc etc.
The question: Does that mean we add the "global-styling" or ALL libraries defined for the Admin theme?
That shouldn't be necessary. There's a risk of it if the libraries we hope to add have it somewhere in there dependency tree, but it's not something fixed.
Possible problem: I don't think just adding the CSS is enough, I can see for e.g. Claro there is a lot of preprocessing of e.g claro_preprocess_input,
Oh! That's a really good point. That will need to be looked at before a just-CSS approach could be recommended.
re #24 I'm not seeing where the suggestions in #21 + #22 simplify anything that's been discussed so far. I see that goes into greater detail (and does so nicely) about how the approach mentioned in #10 would be implemented, but I'm not seeing any time savers here there's still the "but we need to forward the commands from the iframe so that we can run them where the media field is actually rendered." mentioned which was my largest concern regarding effort, and something that Site Studio was never able to get running reliably.
While here, I also wonder how much we need to prioritize "without affecting the main document". Maybe we can just add the admin theme's CSS to the main doc?
We're already pulling in any libraries #attached in a form - including Media Library libraries provided by the module. If we want to fence CSS entirely we'll need to solve it for more than just the media library widget. The media library widget selectors are pretty specifically named and are probably less risky than other core CSS that is already being added via #attached in props forms. The Preview is already wrapped in an iframe so the CSS-contaminateable area is smaller than something like Layout Builder, and largely UI related elements in our control.
Added a suggestion on how to avoid wait()
See MR
This is intentionally added as an SVG document so it can receive CSS styling. This is responsible for the lines that show the relationship between tabledrag items that are seen while dragging. It functions as dynamic element in the UI, not just a background image.
Without the css to conditionally hide/show portions of the SVG this either results in losing the lines altogether or having ALL the lines visible at all times we don't want this 👇 (nor do we want those lines to be removed entirely)
I'm sure there's more work to be done on the Semi Coupled docs, but this is a good point to get other eyes on it then I can address points of confusion and other feedback.
The approach to styling a single tab in MR 8888 is good!
Something I noticed is about the current solution: it only works if the tab contents are one line. Anything beyond that results in the same symptoms that were reported. We should find a solution that works regardless of the tab content length.
bnjmnm → changed the visibility of the branch 3458990-alignment-break-and to hidden.
Good find @ctrladel ! Fortunately, I don't think we need to make the css, js_header, and js_footer properties "optional" because these properties are guaranteed to be present in a Drupal-provided component list. The issue here was the dummy data in sections.ts was incomplete so updating just that should be sufficient.
TY for the screenshots - I added them to the issue summary and this is back to RTBC.
I was pleased enough by the code that I forgot some housekeeping stuff: The issue summary should provide before/after screenshots with the after using the current MR.
The MR itself is RTBC IMO
It may take a bit to address the gitlab thing so changing status as to not confuse any committers - but this is effectively RTBC as soon as the gitlab diff is worked out and cspell runs green
The cspell error in DrupalCI appears unrelated to the MR. As far as the code changes go, this is RTBC but (as one might assume) should not be committed until whatever underlying issue causing the cspell failures is addressed. We can try re-running it later.
It was correctly pointed out in #6 that this is not specific to Olivero, but the issue summary was still specifying that. The solution will likely be inside the off-canvas reset CSS in core/misc/dialog/off-canvas/css/
Good solution and tests, left a bit of feedback on the MR for commenty stuff.
Closed the first MR in favor of the newer solution
Will need tests. We have https://github.com/dmtrKovalenko/cypress-real-events running which I recommend using for the right clicks.
The test can probably compare pointer coordinates to the menu coordinates and allow for a certain range of difference since the location might not be 100% consistent.
If pointer coordinates seems too flaky, the distance from a specific corner of the component with the menu might work. Even just confirming the menu top left corner is inside the component dimensions could work. Pick your fave.