The cookie setup section needs work, but also we miss a follow-up for re-adding review-setup instructions.
This is shutdown now. We succesfully migrated to Github codespaces, but left gitpod setup in place. Thus, marking as done.
I've created a new layer for drupal-cms and starting to test it via:
> npm create nuxt@latest -- -t layer
> How do you propose I can help here?
How is your layer coming along? Do you have some usage instructions, on how it can be installed on top of e.g. the naked or some other setup? Seeing necessary steps there would help me.
I'm wondering whether we want to locally clone the layer into ~/layers/ directory and have it registered that way, or whether we want to use the extends + github:repo.. syntax. I guess for working with it + discoverability the manual clone would be a bit better. Any preferences / opinions on that?
thx, merged then!
There is a change-notice which documents the change. Not sure where we would it else?
I guess we should update the documentation about views at lupus-decopled.org!
ok, I finally tracked the problem down due to how cache-contexts are calculated.
Lupus CE renderer correctly adds this cache contexts:
'url.query_args:_content_format',
'url.query_args:_select',
But the problem is the _content_format is actually not changed via the query parameter, but lupus_decoupled_ce_api actually changes it via the request attribute. That means, lupus_ce_renderer should expose this is a new cache context which works like getContentFormatFromRequest().,
> $default_content_format = $request->attributes->get('lupus_ce_renderer.content_format', $content_format_settings);
anybody, thank you!
Well, this "plugin type" is not an existing concept, this is just how I called it. The plugin IDs are custom_elements_page and custom_elements_block respective. But since with custom elements for a drupal-view it's pretty clear you are using one of those, repeating the custom_elements prefix seems stupid and makes just long ugly names. That's why I'd think we better remove it.
But given that, yeah, there is no better api to do that.
Discussed with usernamee that we do another final test round after merge, so moving on! Thanks for the review!
Thank you for the review! Merged!
During prototyping I identified the following challenges:
* CORS issues. When loading static JS from another domain, we need to make Nuxt provide suiting CORS headers
* Browser JS code cannot use helpers exposed by JS modules like Vue's h(). We could expose needed helpers manually from Nuxt
I had the opportunity to dive into this a bit at DrupalDevDays. XB already does a great job rendering into components, so we should be able to re-use that and build upon it perfectly fine. There is the "hydated-component-tree" which we could use, see ComponentTreeHydrated. Or, we simply use the generated render-array consisting of nested components and blocks, and process into the tree of custom elements. (Similar to what we do with layout builder). Code-components are rendered as astro-islands, so could be output as astro-island component also.
I clarified with balint brews that we do not need to handle auto-saving storage or something, preview is going to work differently anyway. Then we should be able to do all this as simple CE-field formatter.
Created 📌 First-class experience builder support Active for the overall plan.
borisson_ → credited fago → .
implemented the proposed solution -> added MR and a draft change notice
created MR, draft change notice and adapted test. Ready for review.
Added MR. I made the attribute be an empty string instead of NULL, so it's explicit. a NULL attribute would end unset and a possible default value be applied, so with the empty string it should be more clear.
Also extended test-coverage and refactored the test case a bit.
Not sure we can easily hide the option, I don't think we have the control over the block configuration form without lots of kungfu. Maybe the most pragmatic way would be to simply only send the "title" attribute if the display option is checked.
thx, merged then!
Also created follow-up for 📌 "display title" option for view blocks does not work Active .
> When no view title is set (which is the default) an empty title attribute is rendered. What about using the view's name as fallback?
hm, interesting. I'd leave it up to the frontend to make this fallback if desirable. When we set the view name it's hard for the frontend to differentiate from a real title, so it should be done there imo. When no view title is given/set, an attribute seems ok / honest to me.
> All view formats other than Custom Elements don't show any results. Should we limit the options?
Sounds good, but I don't know whether this is feasible. I'd suggest this is a follow-up improvement + applies to pages also.
> The Display Title option in the block configuration does not have any effect
Good point. So maybe we should hide that option? Imo, we could do this as a follow-up improvement.
ok, I've added the test coverage also. I shortly tried doing a kernel test, but somehow the setup made troubles, so I extended the pre-existing functional test for the page instead. Next, I added follow-ups for the related issues and improvements I found and mentioned before. So this should be ready now.
This is at least major. This has the potential to confuse people + might break actual sites.
I can confirm this is fixed now. With the fix in core, this works, so let's mark this as fixed also!
ok, some more updates:
* Fixed block title overrides
* Fixed php8.1 compatibility
* Add config schema
Now, we just need to add test. Then this should be complete.
ok, I got things working now. I refactored the code such that both display plugins, page and block, use the regular views control view for rendering. Before we have been bypassing it, instead of implementing render(). That's fixed now.
One thing that does not yet work, is the overridden title.
Then, we probably want to improve the custom-element tag to append the display id. But since this is a change for the page also, I think this should be its own change notice.
Also, this still needs a test. Meanwhile, this could be already reviewed.
With site templates coming up, decoupled site templates will be a thing, at least we and another company I know would like to create them. While possible with composer, it would be a pity if we would have to host the frontend outside of drupal.org. But if this does not get resolved, we are not left with another choice.
Likewise, with design systems coming up, it will be a topic to create the with MIT such that they are portable to React, Vue, ... If this does not get resolved, they would have to move elsewhere also.
Thank you for the pointer! I've commented on the other issue.
For keeping this issue up2date, I'm also crossposting here. I think 🌱 Unify & simplify render & theme system: component-based rendering (enables pattern library, style guides, interface previews, client-side re-rendering) Active would be the perfect first step for going towards a multi-frontend architecture. As I commented at #2702061-114: Unify & simplify render & theme system: component-based rendering (enables pattern library, style guides, interface previews, client-side re-rendering) → we need to have a 2nd-step through to entangle the control flow such that we are able to generate a complete tree of components first and render it in a 2nd step. Once we have that we can also serve it via an HTTP API for a) multi-frontend usage and b) better debugging XP.
Coming over from
📌
Make Drupal core multi-frontend
Active
@catch - Thank you for pointing me to this issue.
Moving it all over to components and solving challenges like getting rid of pre-processors would be an amazing and a big first step for moving multi-frontend. However, there is another related level of complexity in Drupal theming: The back and forth between Twig and Drupal rendering.
It's been a while since, a tried a twig-powered component approach with Drupal. pdureau, could you share you experience on this, I assume this is stilll how it works, i.e. when you build up a page with components? Like a node component rendering fields, and the fields "content" triggering the rendering of a field renderable again, would again render nodes and... - you get the idea. In my experience, that back and forth leads to very complex control flow, that becomes hard to debug when needed.
Instead, I'd love to have a a tree of components generated first, then in a second step have it rendered. When required for performance, it could have some explicit support for lazy-rendering/generating a sub-tree of components, but I think that should be explicit and used only rarely. If we could generate that kind of tree, it would be very hand to debug/see what the frontend is going to output by e.g. exposing that tree via API (optionally expanded). This would aid theming DX quite a bit imo.
Second, a rendered component tree served via API comes with the additional benefit of becoming renderable by different frontends. E.g., this is all what Lupus Decoupled does, make Drupal serve a tree of components via API, so a Vue/Nuxt frontend can pick it up.
IT's merged, but the resulting description is now a weird mix between gitpod and github!? see https://github.com/drunomics/lupus-decoupled-project?tab=readme-ov-file#...
Development in Cloud
GitHub Codespaces
Gitpod testing options
The code looks mostly good now!
However, as discussed with roderik, the remaining todo is that we need to unwrap the resulting custom-element. It seems the views-block integration results into some render-array wrapper, what we need to get rid off, such that the API contract is on the block-level holds.
I think we are pretty much done here, but there is one issue which confused me: Test location. It seems we have some tests under the main module, while others are split in the moduels we are testing. Where should tests go? I think we shall decide on a rule and then make a child-task to make sure it's enforced. Thoughts on the right rule?
Turns out we cannot handle the prefix/suffix proposal with Vue. So we had to adapt the concept and invent a "drupal-ce-container" component to handle vue-processing difficulties.
useernamee → credited fago → .
Thank you! Merged!
Atm, not having translation support in Drupal CMS is quite unfortunate - it's a big, important feature that seems "unsupported" for evaluators.
I'm not sure why adding a specific language would need to be a recipe, configurable or not. The recipe is the ability to have multiple languages and necessary configuration for that. The specific languages can then be added through the regular UI, that seems like a non-issue to me.
I very much agree with that. If we pair this with the sensible defaults as described by breidert:
Translation settings:
Entities Types such as Nodes, Taxonomy Terms, Media Types, will be translatable.
Basic fields such as, Textfields, Textareas, will be translatable
Entity reference fields such as reference fields to taxonomy terms or media types, will not be translatable.
we could quite easily make recipe that turns on translation for content+config+interface. Yes, it's not perfect, but it's starting point + it shows off that Drupal can do it. I think that would be much better than having it "seem unsupported".
jjchinquist → credited fago → .
We decided
> 2. github codespaces - they atm offer 60h per month free -> https://github.com/features/codespaces
is the way togo.
This is getting critical now!
thank you! Reviewed it and edited a bit. Seems good to go now!
However, I did not test it yet. So it still needs a manual test before it's ready for merge!
93475a99 - Make LupusDecoupledBlockRenderer return a CE instead of HTML for some blocks.
I've no clue how/why this relates to this improvement. If the block-layout support has issues, please open a dedicate issue and move the code there. We can also use layout-builder and place the block to make sure it works. That'S the main use-case anyway.
(...) the block rendering should pick up the CE wrapped in the render-array.
I assume this means that you want to do a combination of two things:
NO, neither A or B shall be done here, ideally. We need to make sure the block provides a proper render-array with custom element - as documented - and it all shall work with that. If not, that's separate bugs to fix/open, but for layout-builder it definitely works.
The output of the above render array below "views_build" looks good, if this is the block's render array, it's all good. if the whole thing is the blocks render-array we need to change it, ideally from our views-plugin, to provide only the main-part we need.
Finally, we need to include test-coverage, i.e. a view with block + verified it renders correctly. Best we ship some layout-builder config that embeds it + test it works as it should.
thank you! that looks pretty good now. I've added one remark with this missing value though, please check MR review.
that seems pretty good already, thank you!
A few remarks:
- please check the MR comment
- I am not sure about the /api/lupus prefix also, what about just /api/site-info or /lupus-site-info ?
- We need some configuration form or README that documents how to use it
- We need some basic test coverage before merge!
Works like a charm! Merged, thank you! :-)
Thank you, that looks great!
The test case is not a functional test though, so I moved it into a new Kernel folder. It might even work as unit test, but I did not bother trying / asking for this to become a unit test. That seems to be a minor optimization which should not hold things up. If it works as unit test, it could be improved in a follow-up issue if you or someone likes. But as said, I think it's ok as is!
Testing this now on gitlab1
Thanks!
The MR looks good, agreed!
Some small things:
- https://git.drupalcode.org/issue/lupus_decoupled-3503738/-/jobs/4374454 shows phpcs issues
- I think we should comment on the event-subscriber weight, why is set to 10. It should explain we take care of running after dynamic page cache.
- Then, we need to add test coverage. Could you take care of that? I think we should be able to extend some of the existing tests to also check that the user-session data is returned correctly.
Given there will be more layers, like https://github.com/StirStudios/stir_nuxt_base/tree/release/nuxt-ui-lupus, I think we should add support for adding nuxt-layers via some optional environment variable to gitpod-links. Then we can easily build links which auto-add the layer as needed.
For example an optional variable FRONTEND_NUXT_LAYER=github:StirStudios/stir_nuxt_base#release/nuxt-ui-lupus could add the respective nuxt-layer to a nuxt-based frontend. I'm adding a sub-task to discuss and clarify details there.
Thank you! but as pointed out at the parent task, I think this should go into lupus_decoupled!
@glynster:
Thank you, but you put it into the wrong module. As said before I think the right place is here:
> To clarify what I think to be the right place: lupus_decoupled_ce_api module
Thus, it needs its own event-subscriber class like EventSubscriber/CurrentUserResponseSubscriber.php
Reason: LupusCeRenderer is a generally usable module and NOT an opinionated setup. The opinionated setup is here in lupus_decoupled, so this is the right place to add optional stuff that we think make sense, but are no existing core feature - like this one.
Could you move it over? Then we need to extend some of the test-cases to verify the output is right also.
thank you! I reviewed the PR and left a couple of remarks there, please check.
Also, please always provide Gitpod-test links for your branches when creating a PR. You need it anyway to test it yourself, so please be so kind and provide the link to reviewers as well.
thx, tested both PRs via gitpod. Much better! Mattia reviewed and approved already.
Merged, thank you!
https://github.com/drunomics/lupus-decoupled-nuxt3-demo/commit/37810a54a...
https://github.com/drunomics/lupus-decoupled-nuxt3-demo-shadcn/commit/28...
I created the list of unified hosting options: https://github.com/drunomics/lupus-decoupled-website/commit/4b86ecf35bfb...
What's left is only
* #3490828: Add a Drupal introduction page
* Add page of "starters" below nuxt
which each have a dedicated issue. Given that, I think we can the current state of the docs "solid", call this done and add those two pages in the mentioned issues as follow-up.
Works all fine! Login & Logout, node editing, all good.
I merged changes from the naked repo into shadcn repo: https://github.com/drunomics/lupus-decoupled-nuxt3-demo-shadcn/commit/04...
Testing now.
A new core release with the fix is out, I think we can call this fixed!
Successfully tested the MR, it resolves the issue while responsive preview integration still works.
ContentSecurityEventSubscriber sets both frame-ancestor and frame-src CSP headers, but only frame-src is necessary to make responsive-toolbar work. the other one seems out of scope and creates this issue. Since there is no reason to set it in this module, imo it's best to remove it.
it seems the header is set by
modules/lupus_decoupled_responsive_preview/src/EventSubscriber/ContentSecurityEventSubscriber.php
Could we improve the naked demo with some little css added by default to forms, maybe just some margins?
For the shadcn demo, could we configure tailwindforms globally somehow such that the form looks decent.
great!
To clarify what I think to be the right place: lupus_decoupled_ce_api module + EventSubscriber/CurrentUserResponseSubscriber.php that works like https://git.drupalcode.org/project/lupus_ce_renderer/-/blob/2.x/src/Even...
thanks! recipes should be able to require v3.
also, please add your patch as merge requests, so we can have the automated test suite run + add some short addition to the tests to ensure it's added correctly.
Thanks! However the patch has a problem with a cachability, without adjusting cache-metadata this can become cached wrongly, e.g. consider dynamic-page-cache using the same cache for multiple users.
To fix, I think we should add this late after dynamic page cache, as we do with other dynamic content (local tasks) already: https://git.drupalcode.org/project/lupus_ce_renderer/-/blob/2.x/src/Even...
Thus, when we add here a similar responsesubscriber and same wait for adding session information, we should be good. Could you change it like that?
Verified, the error is gone!