- Issue created by @jessebaker
- First commit to issue fork.
- First commit to issue fork.
- ๐บ๐ธUnited States hooroomoo
hooroomoo โ changed the visibility of the branch 3452512-edit-2 to hidden.
- ๐บ๐ธUnited States hooroomoo
Started on edit-3 branch which is off the most recent 0.x that has โจ [MR Only] Edit any component prop, powered by a new FieldForComponentSuggester service, which will power the JS UI Fixed . Didn't get very far yet, currently the branch is just rendering the values from field.feld..node.article.field_xb_demo which is hard-coded in layout-default.json. Not sure if there's an endpoint that exists yet that I can fetch the tree and props/model from.
- ๐ง๐ชBelgium wim leers Ghent ๐ง๐ช๐ช๐บ
See gitlab MR description for more details...
There isn't any ๐
- Assigned to hooroomoo
- ๐บ๐ธUnited States hooroomoo
hooroomoo โ changed the visibility of the branch 3452512-noop to hidden.
- Status changed to Needs work
5 months ago 9:00am 11 June 2024 - ๐ง๐ชBelgium wim leers Ghent ๐ง๐ช๐ช๐บ
@hooroomoo shared a screenshot in Slack, attaching here:
๐ Unfortunately, I canโt get to that point locally โ maybe because of
$ npm run build > vite-template-redux@0.0.0 build > tsc && vite build nsrc/features/panel/ContextualPanel.tsx:30:35 - error TS7031: Binding element 'htmlString' implicitly has an 'any' type. 30 const BadHtmlParserComponent = ({ htmlString }) => { ~~~~~~~~~~ src/features/panel/ContextualPanel.tsx:43:62 - error TS2345: Argument of type 'string | undefined' is not assignable to parameter of type 'string'. Type 'undefined' is not assignable to type 'string'. 43 const responseAsDocument = new DOMParser().parseFromString(response.data, 'text/html'); ~~~~~~~~~~~~~ Found 2 errors in the same file, starting at: src/features/panel/ContextualPanel.tsx:30 wim.leers at MacBookPro-WimLeers in ~/core/modules/contrib/experience_builder/ui on edit-3*
- ๐บ๐ธUnited States hooroomoo
Ah yeah I see there are TS errors that cause the build to fail. But if you run in development mode
npm run dev
you should be able to reproduce it. I will fix the build errors though. - ๐ซ๐ฎFinland lauriii Finland
We still need to wait for new endpoints to actually make changes to the backend so this is still in early stages.
What are the missing endpoints? Are there Drupal.org issues for these endpoints already? Can we link them here for clarity?
- ๐บ๐ธUnited States bnjmnm Ann Arbor, MI
Updated IS with details of how the form is being generated.
- ๐บ๐ธUnited States hooroomoo
Current state it only has the text prop rendering right now:
- Status changed to Active
5 months ago 6:08pm 13 June 2024 - ๐บ๐ธUnited States hooroomoo
Radix tooltip that shows unique data to prove JSX-ified drupal form can be integrated with our frontend
- ๐บ๐ธUnited States hooroomoo
This is using Drupal's autocomplete behavior
- Status changed to Needs work
5 months ago 3:56pm 20 June 2024 - ๐ง๐ชBelgium wim leers Ghent ๐ง๐ช๐ช๐บ
Marking for the remarks I posted.
Plus, @effulgentsia just requested that "undo" support works correctly even when the autocomplete is used โ that's indeed worth verifying that it works correctly.
After my remarks + @effulgentsia's is addressed, please assign this to @bnjmnm. When he gets back from PTO next week, I think he'd be the best person to extract pieces that can land today. No pieces that call/reuse
TwoTerribleTextAreasWidget
should land, and instead we should work on the next steps under in ๐ฑ [META] Early phase back-end work coordination Active . - Assigned to bnjmnm
- ๐ง๐ชBelgium wim leers Ghent ๐ง๐ช๐ช๐บ
I'm working to unblock @bnjmnm here by finishing โจ Allow specifying default props values when opting an SDC in for XB Fixed and then landing ๐ [PP-1] HTTP API: update /xb-render-component/{component_id} to use Component config entity's default values Postponed โ together, they will ensure that it becomes feasible to edit components without the horrors of
TwoTerribleTextAreasWidget
๐งโโ๏ธCrucially, that will allow @bnjmnm to fix the one inescapable bug he's been running into here: dragging in a new component onto the canvas results in fatal responses from the server to the client's request to preview an additional component (and then subsequently edit it).
- ๐ง๐ชBelgium wim leers Ghent ๐ง๐ช๐ช๐บ
๐ Add some basic example SDCs Fixed landed โ this MR can now be rebased and should become ~240 LoC smaller.
EDIT: I see that @bnjmnm already removed these ๐
@bnjmnm I'll get the PHPStan things out of the way for you. ๐
- ๐ง๐ชBelgium wim leers Ghent ๐ง๐ช๐ช๐บ
PHPStan &
phpunit
CI jobs are now passing.I unfortunately can't locally see your awesome work in action, because I get this:
$ npm run build > vite-template-redux@0.0.0 build > tsc --noEmit && vite build error TS6305: Output file '/Users/wim.leers/core/modules/contrib/experience_builder/ui/vite.config.d.ts' has not been built from source file '/Users/wim.leers/core/modules/contrib/experience_builder/ui/vite.config.ts'. The file is in the program because: Matched by default include pattern '**/*' Found 1 error.
I wondered if something was wrong about my
node
setup (I'm still on version 18 ๐ ), but then I discovered that very same error appears on the Cypress CI job: https://git.drupalcode.org/project/experience_builder/-/jobs/2082413 - ๐บ๐ธUnited States bnjmnm Ann Arbor, MI
I'm getting the same dang thing as #26 if I run
npm run build
error TS6305: Output file '/Users/austin.powers/core/modules/contrib/experience_builder/ui/vite.config.d.ts' has not been built from source file
It will, however, work with the
npm run drupaldev
I added as part of this MR. - Issue was unassigned.
- Status changed to Needs review
4 months ago 1:18pm 12 July 2024 - Assigned to wim leers
- ๐ง๐ชBelgium wim leers Ghent ๐ง๐ช๐ช๐บ
Reviewing! ๐คฉ
The MR is not running all tests currently, due to ๐ CI: do not run PHP-only jobs if an MR only changes the UI Active . I just merged a follow-up MR that fixes that problem: #3460778-6: CI: do not run PHP-only jobs if an MR only changes the UI โ . Merging in upstream to get all tests running in this MR ๐ ๐
- Assigned to bnjmnm
- Status changed to Needs work
4 months ago 5:18pm 12 July 2024 - ๐ง๐ชBelgium wim leers Ghent ๐ง๐ช๐ช๐บ
Both @jessebaker and I posted our reviews :)
- Issue was unassigned.
- Status changed to Needs review
4 months ago 9:06pm 12 July 2024 - Assigned to jessebaker
- ๐ง๐ชBelgium wim leers Ghent ๐ง๐ช๐ช๐บ
I created the critical follow-up ๐ Evolve component instance edit form to become simpler: generate a Field Widget directly Fixed to make this no longer rely on
TwoTerribleTextAreasWidget
. See that issue for details.๐ [PP-1] HTTP API: update /xb-render-component/{component_id} to use Component config entity's default values Postponed is a pre-existing issue that will simplify a narrower piece of this issue.
(This issue should land first, because it introduces a lot of infrastructure critical to deliver the envisioned UX, and simplifying something hacky that works, is much simpler than requiring completeness or perfection here.)
From a back-end POV, this is ready to land (to unblock subsequent issues).
Assigning to @jessebaker, because he just merged โจ Close submenu on drag in primary menu Needs work , which conflicts with this. Also to address the last N bits of front-end related feedback:
- missing docs: https://git.drupalcode.org/project/experience_builder/-/merge_requests/5...
- extraneous comma: https://git.drupalcode.org/project/experience_builder/-/merge_requests/5...
- โฆ presumably a last review pass just like I did ๐ค
- Status changed to RTBC
4 months ago 10:41am 15 July 2024 - ๐ง๐ชBelgium wim leers Ghent ๐ง๐ช๐ช๐บ
Signaling this is ready, and that @jessebaker should feel comfortable fixing himself whichever nits he still spots, to allow @bnjmnm (and all of us, really) to move on to the next steps! ๐
- ๐ง๐ชBelgium wim leers Ghent ๐ง๐ช๐ช๐บ
One more follow-up issue, which is the next issue @bnjmnm should work on: ๐ End-to-end test that tests both the client (UI) and server Active . Ben and I already discussed this last Thursday. This would have prevented ๐ TypeError on a clean installation Closed: duplicate .
-
Wim Leers โ
committed c162272b on 0.x authored by
hooroomoo โ
Issue #3452512 by bnjmnm, Wim Leers, hooroomoo, jessebaker: Add...
-
Wim Leers โ
committed c162272b on 0.x authored by
hooroomoo โ
- Issue was unassigned.
- Status changed to Fixed
4 months ago 12:57pm 15 July 2024 - ๐ง๐ชBelgium wim leers Ghent ๐ง๐ช๐ช๐บ
It's in! ๐ข
Next steps unblocked:
- ๐ฆ๐บAustralia larowlan ๐ฆ๐บ๐.au GMT+10
This feels like a lot of smoke and mirrors. I understand we're trying to have a demo up for Barcelona, but a lot of the code in the MR makes me uncomfortable. I realise I was on leave and the ship has sailed but I would expect to see a lot more follow-ups than #36
- Assigned to bnjmnm
- Status changed to RTBC
4 months ago 8:10am 16 July 2024 - ๐ง๐ชBelgium wim leers Ghent ๐ง๐ช๐ช๐บ
There's no smoke and mirrors, it actually works. But it is very complex. It's the price for the product decisions that:
- we'd use โจ JSON-based data storage proposal for component-based page building Active , which means Drupal field types will the contain values for SDC props
- most importantly: the existing Drupal ecosystem must be brought along for the ride, which includes being able to use existing field types and field widgets
Note that that second decision does not mean that only existing field widget plugins can provide the input user experience. It just means that that this is the default input UX; in the future we'd want to add support for more optimized widgets. (Paraphrasing what IIRC @lauriii told me a number of weeks ago, when I voiced concerns similar to yours. I do think that )
You're absolutely right that #36 is not the full set of follow-ups. We don't know the full set of follow-ups yet. This landed because it's a big leap forward, not because it is a polished whole! We know there's a lot of things left to be done, but this having landed is precisely what will allow us to discover unknown unknowns and convert them to known unknowns.
All the above being said: reopening this and assigning to @bnjmnm, to ask him to create follow-up issues for the things that he already identified as logical next steps. He's the only one who can do that because he's the only one who so deeply understands how this works.
- ๐ฌ๐งUnited Kingdom catch
we'd use #3440578: JSON-based data storage proposal for component-based page building, which means Drupal field types will the contain values for SDC props
This still has major unresolved questions around it. It may or may not affect the editing experience, but for example the 'field union JSON' proposal might well provide alternatives, while fulfilling the existing product requirements.
- Issue was unassigned.
- Status changed to Fixed
4 months ago 10:55am 26 July 2024 - ๐ง๐ชBelgium wim leers Ghent ๐ง๐ช๐ช๐บ
#39: Yes, there are still plenty of unresolved questions. But given the early stages of this project, I think that's unavoidable. Especially because we're working based on 64 concise product requirements and @lauriii is working to get designs
Regarding smoke and mirrors: I think โจ Contextual form values need to be integrated with Redux Active and its follow-ups ( โจ Include component props form in undo Needs review , ๐ฑ [META] Redux sync on ALL prop types, not just ones with a single [value] property Active , more to come) are getting rid of the smoke (and hence revealing there are no mirrors, except maybe broken ones โ sorry, the temptation was too great ๐). Yes, it will all need to become clearer. But we're iterating towards more clarity, and to discover ASAP whether what each individual thinks (based on their expertise) should work, actually works.
Yes, there will be many more follow-ups. But no, they're not all created ahead of time. Yes, that makes it more difficult for others to see where this is going exactly. But we don't know where this is going exactly either!
It's obvious to everyone that XB is nowhere near production-ready (and nowhere near many other things), but we are developing this in the open from day 1.
๐ฑ UX design tracker Active is growing steadily, so it's gradually becoming easier for everyone, including us working on XB full time, what the exact intended end goal is.
Automatically closed - issue fixed for 2 weeks with no activity.