🇺🇸United States @bnjmnm

Looks awesome in Chrome! But in Firefox (my default), the styling gets a little messed up

🤔 I checked this out in Firefox and spotted one thing in the screenshot more (the media titles were wrapping instead of elipsising...) but I'm not entirely sure that would explain how herky jerky that screenshot was... This is how it looks for me now in FF

If you're still having problems, would you be OK looking in the FF inspector and seeing if there are any styles (or lack of) that seem to be making the most nuisance?

Yes, I think we need to revisit this. I'm actually surprised we've managed to get this far with XB with this existing order of operations. It seems fundamentally incorrect. We're using AJV to validate, which means our validation is based on the json schema, which means the thing we need to validate is the prop value, but we don't have a prop value until we run the transforms. Prior to running the transforms, we don't have a prop value, we only have a form input value, and form input values are allowed to be anything so long as the transforms turn them into a valid prop value.

The transformed versions are getting validated, but they're still valid. After digging into the implementations, the format requirements are more relaxed than I'd assumed.

If it is validating entity:node/2

• ✅format: uri passes /^(?:[a-z][a-z0-9+\-.]*:)(?:\/?\/)?[^\s]*$/i (the colon indicates a scheme and the slash indicates a path)
• ✅format: uri-reference passes /^(?:(?:[a-z][a-z0-9+\-.]*:)?\/?\/)?(?:[^\\\s#][^\s#]*)?(?:#[^\\\s]*)?$/i (uri-reference is even more forgiving)

This could be made more robust with the use of pattern: but that alone would result in the scenario mentioned by @effulgentsia where the AJV validation does fail

I'm good with this approach, I think it's more accurate to see this as a stale @todo vs functionality that needs to be changed.

wim leers → credited bnjmnm → .

wim leers → credited bnjmnm → .

Updated issue summary to reflect that is this not referring to a Dropdown list (it is not a list of select menu options) nor is the issue necessarily related to scrollability (there are several ways this can be addressed)

I'm also postponing this as it might be redundant in light of 📌 Style Autocomplete suggestions Active - based on what was reported, this should only require setting the autocomplete collision logic Drupal.autocomplete.options.position = {collision: 'flip'}; which I've already done in that issue's MR. I'll keep this issue postponed instead of closed in case #3529623 has trouble getting in, in which case we can do a targeted MR here with the Drupal.autocomplete.options.position = {collision: 'flip'}; that allows the suggestions to show up regardless of the input's proximity to the bottom of the viewport.

Further, I can't imagine not showing a hidden dependency, that would be a huge point of confusion.

I'm inclined to agree. This issue was filed because

• there were comments in core indicating hidden modules exclusion from "depends on" was the intended functionality, but the code was failing to achieve that.
• Giving the above finding it's own issue make it possible to unblock progress on a the more important issue where the above happened to be found

I was leaning towards keeping things as-is even when this issue was initially filed, and considering 5 additional years have since passed since I'm not sure there's anything that needs changing aside from possibly pruning the @todos

Ideally, the component should not fail to render when a required field is left blank.

I disagree, if a component has an invalid prop, it shouldn't be rendering. That said, the. component should do a better job informing the user why it isn't rendering, which we will address here:

📌 Invalid prop errors should be detailed in Component preview Active

bnjmnm → created an issue.

wim leers → credited bnjmnm → .

Re #11 There were styles for this but it looks like there were some conflicts with focus-visible vs visible but this should be good now.

FB is addressed as far as I can tell so I'm merging to get _none working properly again. If there's lingering bits happy to address in a followup.

avpaderno → credited bnjmnm → .

smustgrave → credited bnjmnm → .

griffynh → credited bnjmnm → .

The two PATCH requests to component-instance is due to the endpoint set to forceRefresh: true, which was added to fix the issue of vanilla Drupal behaviors not re-running if a component form is revisited. (the most common symptom was leaving then re-loading a component with a media library widget would result in the widget JS not working)

Setting forceRefresh: true provides a redux-cached version of the form until the new version is available, then the updated data (including selectors necessary for Drupal Ajax to work) replaces it.. Apparently this setting also makes additional, unnecessary requests. In the MR I updated the criteria to force a refresh if

• The query string changed (which is very expected)
• The last query string came from a different component.

Some manual testing is needed to ensure there aren't scenarios where a re-opening of a props form doesn't force a refetch.

bnjmnm → made their first commit to this issue’s fork.

bnjmnm → created an issue.

The mr 3526866-pretty-major-regression updated some CSS and here's a comparison of the MR to HEAD.

Alright lets see if this eases some e2e angst.

Now that I can run these tests locally (my site was not in a /web folder but with the recent change it works great), I spotted a few additional things that I imagine are quite easy to address.

Needs signoff that @hooroomoo can grant + some of the test improvements need another set of eyes as they were added after the RTBC

bnjmnm → created an issue.

Added a handle-none-option MR with test that should address #15

The final form element is de-Radix'd, which should make things easier & more stable 😎

bnjmnm → created an issue.

The MR in 📌 Create a test module that adds color, machine name, and dropbutton elements to the props form Active includes a test module with color component which can be cherry picked into this.

We should first address 🐛 Autosave publish process does not acknowledge pathauto deactivation Postponed before proceeding here. Once that is fixed, this issue might be completely resolved, too.

Argh - this must have been due to changes that occured during the longer-than-typical review cycle. Filed a new bug for what was found 🐛 SDC Required image can't be removed without error Active . If there's interest in keeping this open until that is fixed NP, but it will be easier for me to get it addressed with a fresh issue scoped to those specific symptoms.

bnjmnm → created an issue.

bnjmnm → created an issue.

bnjmnm → created an issue. See original summary → .

Clearly a problem is occurring in #7 but it isn't one I'm running into (see this video) + this and the e2e test mentioned in #5 demonstrate that "Handl[ing] media image fields on page data form" currently works.

The error in #6 looks like it's coming from OpenAPI validation, which should obviously be addressed. If anyone currently experiencing it can either update this issue summary or create a new issue targeting that specific bug

bnjmnm → created an issue.

No longer relevant as this was addressed by other work. There is confirmation of this in the test 'Can open the media library widget in an xb_page props form' in media-library.cy.js (this test does more than just open, it adds / removes etc.)

Addressed elsewhere. See field_xbt_language.cy.js

Addressed in other issue

This was addressed a while back. The reported error is from Radix due to not meeting its option requirements. Tested manually to make sure this isn't a problem with metatag and it is fine.

In the past week, we also stopped using Radix for our select elements which should make it even less of a concern.

Overview

• Add text-based test cases to xb_test_state_api module.
• As part of getting those tests to pass Make the text components use native elements instead of radix
• Even if the text inputs can't look identical, it should look pretty close. More formal designs will be provided in the future so there's no need to be pixel perfect if it prevents the functionality from happening.

Proposed resolution

Manual Test

Enable xb_test_state_api module and try the inputs added in this MR

Media Library and many aspects of JS/AJAX iwork and there's considerable e2e test coverage confirming this.

It's quite possible there are use cases that have not been addressed yet, but there's definitely not a blanket omission of AJAX/JS integration. If specific problems arise, they are best addressed as targeted issues with tight scope so the specific symptom can be addressed

bnjmnm → created an issue.

bnjmnm → created an issue.

bnjmnm → created an issue.

Thanks for all the reviews etc on this lil' dragger.

Ty @hooroomoo for the review

I have a proof of concept from last year that successfully does this, and the CK team + several committers were in support of the approach. The biggest part will likely be finalizing ✨ Add an API for importmaps Active , after which most of the CK changes are not too intense.

I also created a doc that details the approach and provides steps for several contrib modules on how to approach it

Re #10its not yet possible to get selects to wrap on long options in FF, but it looks much better now. The Chevron being there makes it more apparent it's a deliberate truncation.

When FF supports the select style pseudos, this will look even better

Thx for reviews everyone

avpaderno → credited bnjmnm → .

Took care of the merge conflict and it looks like all other reviews were good with the changes so I'll go ahead and commit.

bnjmnm → created an issue.

Radix / 0.x

Native / this MR

Despite adding a bunch of additional testing the end result is fewer LOC! +341 −345

Thanks for the reviews @hooroomoo & @balintbrews

bnjmnm → made their first commit to this issue’s fork.

bnjmnm → created an issue.

Between this and some other recent changes, these can even be unskipped. Nice work @penyaskito

The is always unchecked part is addressed by
📌 Confirm Semi-coupled form elements can work with State API visibility Active , the other part will likely be fixed with 🐛 Autosave publish process does not acknowledge pathauto deactivation Postponed as mentioned in #4

bnjmnm → made their first commit to this issue’s fork.

bnjmnm → created an issue.

I had this postponed on the State API checkboxes issue becausei I can’t manually reproduce the problem without the State API working with that widget. Based on the above it sounds like this or something else representative of the situation can be reproduce me - could the issue summary be updated with these steps?

It's possible I duplicated this issue with 🐛 Autosave publish process does not acknowledge pathauto deactivation Postponed , but not fully sure as the IS here specifies the issue is due to pathauto creating an alias when a pattern isn't defined for the entity type, while #3526130 is specifically about the publish process not acknowledging the "Generate automatic URL alias" being unchecked.

If they are in fact the same issue reported differently please note the issue summary in #3526130 already provides a 3 line solution that works, but notes there's probably a cleaner way to implement it (perhaps in a hook that specifies module: 'pathauto'

or maybe there are two separate problems?

At least as-reported this issue is different. The problem reported here is specifically about the publish process not respecting the "Generate automatic URL alias" setting, while issue #3525070 states the the problem is a failure to do the desired "Pathauto should skip generating a new path when there is no pattern"

I wouldn't be surprised if the solution to #3525070 occurs in the same place as the solution to the problem reported here, but there's not yet enough info to consider these issues duplicates.

Approved the MR, still needs approval from /src/ and /src/Entity/

Also noticed #15.2 was already discussed but probably worth conclusively answering 1 and 3 even if the answer is sort of implied already just to be sure we're properly acknowledging every review point

Yep the moving / scrolling seems to be one of the more common intermittent issues here. good find.

bnjmnm → made their first commit to this issue’s fork.

@gábor hojtsy the scope of this issue includes getting the pathauto widget functional. With this MR. The form elements in that widget work as expected and send the correct data for submission. The autosave data is aware of pathauto being enabled/disabled and the value of the custom alias.

However, the publish process is not processing these values correctly. This.is out of scope for this widget-targeted issue. Even if it's out of scope, I figured it can't hurt to offer an additional draft MR 3481719-states-api-with-targeted-pathauto that takes the in-scope MR and adds a few lines to ApiAutoSaveController to get pathauto working

 if (get_class($entity->path) === 'Drupal\pathauto\PathautoFieldItemList' && $auto_save['data']['entity_form_fields']['path[0][pathauto]'] === FALSE) {
          $entity->path->__set('pathauto', 0);
        }

I believe this is necessary because the publish process updates fields, but path is being updated as an entity property.
I suspect there are more holistic solutions available so the 3481719-states-api-with-targeted-pathauto branch should probably not be part of this issue's commit, so I created 🐛 Autosave publish process does not acknowledge pathauto deactivation Postponed , which can use the solution I already found as a starting point.

bnjmnm → created an issue.

The Radix version on 0.x

The native version in this MR

I have some MR feedback. I think we can reduce the total renders of the Path widget component by using refs for some items managed by state, but I don't have time ATM to confirm that yet. Will come back to that shortly.

• The MR capabilities are a variantion Pathauto does, and as a result conflicts. It that module is enabled, it will ultimately override whatever is created by this, but confusingly ly, even if the "Generate automatic URL alias" checkbox is filled, the disabled URL alias field will populated with the autogenerated string. (note that this checkbox does not yet work for everyone - I tested this with a branch from issue #3481719 which gets the State API working). I believe pathauto might be bundled with some systems that use experience builder, so we'd need to account for this
• Re #15.2 "And what about SDC props that use type: string, format: uri-reference" is this a concern here? I believe those formats use the widget for link fields, not path fields.

bnjmnm → made their first commit to this issue’s fork.

This looks good and there are enough related followups happening to clean up any loose ends in MRs with a more manageable number of files changed.

Thanks @quietone (and apologies to @geoffreyr ) I may have spoke too soon in #7 . I'm glad you responded as it motivated me to double check my findings and it turns out my IDE was delivering incomplete info.

There is in fact a Core API and it's fine to have this as part of the drupal-apis → guide.

Something to consider adding to the intro page → is an explanation of what this API does vs the Token module. More often than not, I wouldn't want this kind of pre-emptive clarification in a core API guide, but in this case it's an incredibly popular module that makes use of this identically-named API so it might be beneficial.

bnjmnm → made their first commit to this issue’s fork.

Everything else on the APIs page → is a core api - it works without any additional modules. I'd prefer a module API not be on a core documentation page, but if there are strong feelings to the contrary it should ad least be incredibly clear, probably on all pages, that installing the contrib Token module is necessary for any of this to work.

Feedback addressed

Moved from root section to documentation

Moved from root section to documentation

Moved from root section to documentation