Overview

Part of #3463842: [META] Redux sync on ALL prop types, not just ones with a single [value] property
This will reduxify all of the single value inputs that appear in the SDC Test All Props form (this does not include props that exist in the SDC but do not appear in the form)

Proposed resolution

User interface changes

CommentFileSizeAuthor
#10 3473155-9-integer_enum.patch2.8 KBwim leers

Issue fork experience_builder-3473155

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

bnjmnm created an issue. See original summary.

bnjmnm opened merge request !282

bnjmnm’s picture

bnjmnm
he/him
Primary language English
Location Ann Arbor, MI
commented
Assigned: bnjmnm » Unassigned
Status: Active » Needs review
bnjmnm’s picture

bnjmnm
he/him
Primary language English
Location Ann Arbor, MI
commented
Parent issue: » #3463842: [META] Redux sync on ALL prop types, not just ones with a single [value] property
kristen pol’s picture

kristen pol
she/her
Primary language English
Location Santa Cruz, CA, USA
commented

@bnjmnm Let me know if there is a way to manually test this.

bnjmnm’s picture

bnjmnm
he/him
Primary language English
Location Ann Arbor, MI
commented

Refer to the new e2e tests in prop-types.cy.js and perform those steps manually
Itpretty much boils down to adding the sdc test all props component, and with the exception of the Media Library/Image widget at the bottom, making a change to each field and confirming there is no crash and the preview updates accordingly. See the e2e test file for limitations we are already aware of and note that no validation (min/max, required, regex) etc is currently in place. This will be added in a separate issue.

kristen pol’s picture

kristen pol
she/her
Primary language English
Location Santa Cruz, CA, USA
commented

Thanks 🙏 I’ll see if someone can jump on this.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Assigned: Unassigned » jessebaker

I posted an initial review, but I think this especially needs input from @jessebaker.

wim leers credited jessebaker.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Assigned: jessebaker » bnjmnm
Status: Needs review » Needs work
Related issues: +#3472176: String props that are integer values aren't treated as strings
StatusFileSize
new 3473155-9-integer_enum.patch2.8 KB

This is a big leap forward! 👏

In manual testing, this AFAICT does work fine for type: string, enum: […] too? But I defer to @bnjmnm whether to include or exclude that here.

That'd allow #3472176: String props that are integer values aren't treated as strings to be closed too 🤞

bnjmnm’s picture

bnjmnm
he/him
Primary language English
Location Ann Arbor, MI
commented
Assigned: bnjmnm » Unassigned
Status: Needs work » Needs review

Threads addressed, and I'll address #10 in a separate issue - moving off the string assumption requires additional logic not in this issue, and I think it's more likely to get a quality review if it isn't diluted by the many complex changes already in this issue's MR.

kristen pol’s picture

kristen pol
she/her
Primary language English
Location Santa Cruz, CA, USA
commented

I have just tested this MR branch with our WIP updates from:

https://git.drupalcode.org/project/demo_design_system/-/merge_requests/53

and it doesn't fix the hanging props form (on components with images) that was reported in:

#3472900: XBEndpointRenderer & processResponseAssets() do not support `ajaxPageState` ⇒ duplicate CSS/JS loading

Maybe it shouldn't fix it? There are so many issues bouncing around that it's hard to know.

kristen pol’s picture

kristen pol
she/her
Primary language English
Location Santa Cruz, CA, USA
commented

I can confirm that this fixes the textarea props.

bnjmnm’s picture

bnjmnm
he/him
Primary language English
Location Ann Arbor, MI
commented

and it doesn't fix the hanging props form (on components with images) that was reported in:

Images are not single-value props (it is an object with path, alt, width, height) and not covered here. When in doubt (and the doubt is reasonable) look at the e2e tests and if you don't see tests added it's safe to assume it's not in the scope of the issue.

kristen pol’s picture

kristen pol
she/her
Primary language English
Location Santa Cruz, CA, USA
commented

Thanks for the clarification and pointers 👍

jessebaker’s picture

jessebaker commented
Assigned: Unassigned » wim leers

Approved - and love that this resolves the flakey components-slots test!

-> @wim leers for backend review

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Assigned: wim leers » Unassigned
Status: Needs review » Reviewed & tested by the community
wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented

Still needs approval from a semi-coupled theme engine code owner.

But given this is the #1 priority for #3454094: Milestone 0.1.0: Experience Builder Demo and that @bnjmnm is the primary author of that anyway … I think a +1 from @hooroomoo or @effulgentsia is overkill in this case.

Plus, per #16 the flakey components-slots E2E test, so … bypassing the need for that approval 🤓

  • wim leers committed 12b7aa98 on 0.x authored by bnjmnm 
    Issue #3473155 by bnjmnm, wim leers, jessebaker: Redux Sync all single-...
wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Status: Reviewed & tested by the community » Fixed

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.