πŸ‡¦πŸ‡ΊπŸ.au GMT+10
Account created on 21 November 2008, over 15 years ago
#

Merge Requests

More

Recent comments

πŸ‡¦πŸ‡ΊAustralia larowlan πŸ‡¦πŸ‡ΊπŸ.au GMT+10

I feel this one is valid enough to add tests for.

\Drupal\entity_test\Entity\EntityTest in core only has a base table, no data table etc.

πŸ‡¦πŸ‡ΊAustralia larowlan πŸ‡¦πŸ‡ΊπŸ.au GMT+10

@xjm - you might be able to edit blocks, but not create them. The block library has operations links for each block. But regardless, this isn't changing that - this is removing the need to access the block library in order to create inline blocks.

i.e this change allows a site to provide a link to create a new block, but without needing to give the user access to the block library.

In a node analogy, allowing the user to create a basic page without giving them access to admin/content.

πŸ‡¦πŸ‡ΊAustralia larowlan πŸ‡¦πŸ‡ΊπŸ.au GMT+10

Not a layout builder subsystem maintainer, but framework manager opinion - this was an oversight from πŸ› BlockContentAccessControlHandler requires access block library permission for update, delete and revisions operations Fixed , which itself was an oversight from ✨ Add Block Content revision UI Fixed and filed almost straight after that went in by @jenlampton.

Confirming that \Drupal\layout_builder\Controller\ChooseBlockController::inlineBlockList does not consider create access when dealing with inline blocks.

πŸ‡¦πŸ‡ΊAustralia larowlan πŸ‡¦πŸ‡ΊπŸ.au GMT+10

Thanks @DamienMcKenna - can we get an MR so we can have gitlab CI? There's no drupalci here anymore

πŸ‡¦πŸ‡ΊAustralia larowlan πŸ‡¦πŸ‡ΊπŸ.au GMT+10

Thanks @DamienMcKenna - can we get an MR so we can have gitlab CI? There's no drupalci here anymore

πŸ‡¦πŸ‡ΊAustralia larowlan πŸ‡¦πŸ‡ΊπŸ.au GMT+10

The new patch uses a limit and sort per #3425888-6: Investigate performance of block_content_post_update_set_owner β†’ which in theory should resolve the performance issue.

I like the 'let's just make them all anonymous (uid 0)' idea because none of block content's permissions (see \Drupal\block_content\BlockContentAccessControlHandler::checkAccess) take the owner into account. But given the update hook is written, I think we should test the performance. I will ask @luke.leber if he can retry this patch with the same site he saw the issue on previously. If it performs OK, I think we may as well leave things in the best state rather than offloading the question to a contrib solution.

πŸ‡¦πŸ‡ΊAustralia larowlan πŸ‡¦πŸ‡ΊπŸ.au GMT+10

We should put it on the project page too

πŸ‡¦πŸ‡ΊAustralia larowlan πŸ‡¦πŸ‡ΊπŸ.au GMT+10

larowlan β†’ created an issue.

πŸ‡¦πŸ‡ΊAustralia larowlan πŸ‡¦πŸ‡ΊπŸ.au GMT+10

I'm not sure I understand.
Are you saying it's not working with this MR or not working in HEAD?
Happy to help either way

πŸ‡¦πŸ‡ΊAustralia larowlan πŸ‡¦πŸ‡ΊπŸ.au GMT+10

May I suggest the entity share module
It is exactly what you want here

πŸ‡¦πŸ‡ΊAustralia larowlan πŸ‡¦πŸ‡ΊπŸ.au GMT+10

larowlan β†’ created an issue.

πŸ‡¦πŸ‡ΊAustralia larowlan πŸ‡¦πŸ‡ΊπŸ.au GMT+10

larowlan β†’ made their first commit to this issue’s fork.

πŸ‡¦πŸ‡ΊAustralia larowlan πŸ‡¦πŸ‡ΊπŸ.au GMT+10

fixed ta, will rebase your branch

πŸ‡¦πŸ‡ΊAustralia larowlan πŸ‡¦πŸ‡ΊπŸ.au GMT+10

larowlan β†’ created an issue.

πŸ‡¦πŸ‡ΊAustralia larowlan πŸ‡¦πŸ‡ΊπŸ.au GMT+10

Woot woot, 11.x here we come

πŸ‡¦πŸ‡ΊAustralia larowlan πŸ‡¦πŸ‡ΊπŸ.au GMT+10

larowlan β†’ made their first commit to this issue’s fork.

πŸ‡¦πŸ‡ΊAustralia larowlan πŸ‡¦πŸ‡ΊπŸ.au GMT+10

Can you share the yml for your view?

πŸ‡¦πŸ‡ΊAustralia larowlan πŸ‡¦πŸ‡ΊπŸ.au GMT+10

I wonder if we need the overhead of the DSL?
Gitlab supports mermaid and we can edit them direct on mermaid.live

πŸ‡¦πŸ‡ΊAustralia larowlan πŸ‡¦πŸ‡ΊπŸ.au GMT+10

I still think this is useful for manual testing MRs without needing to check a branch out locally

πŸ‡¦πŸ‡ΊAustralia larowlan πŸ‡¦πŸ‡ΊπŸ.au GMT+10
πŸ‡¦πŸ‡ΊAustralia larowlan πŸ‡¦πŸ‡ΊπŸ.au GMT+10
πŸ‡¦πŸ‡ΊAustralia larowlan πŸ‡¦πŸ‡ΊπŸ.au GMT+10

larowlan β†’ created an issue.

πŸ‡¦πŸ‡ΊAustralia larowlan πŸ‡¦πŸ‡ΊπŸ.au GMT+10
πŸ‡¦πŸ‡ΊAustralia larowlan πŸ‡¦πŸ‡ΊπŸ.au GMT+10

As seen, we can remove a lot of ignores as well as future proofing by tightening return type-hints.

πŸ‡¦πŸ‡ΊAustralia larowlan πŸ‡¦πŸ‡ΊπŸ.au GMT+10
πŸ‡¦πŸ‡ΊAustralia larowlan πŸ‡¦πŸ‡ΊπŸ.au GMT+10

larowlan β†’ created an issue. See original summary β†’ .

πŸ‡¦πŸ‡ΊAustralia larowlan πŸ‡¦πŸ‡ΊπŸ.au GMT+10

Sounds good to wait for 10.3 - thanks!

πŸ‡¦πŸ‡ΊAustralia larowlan πŸ‡¦πŸ‡ΊπŸ.au GMT+10

We can also write a test that loads the fixtures from UI and validates them AND uses drupalGet to load the controllers and validate them.

Compliance all round πŸ’ͺ

πŸ‡¦πŸ‡ΊAustralia larowlan πŸ‡¦πŸ‡ΊπŸ.au GMT+10

There's no output from composer outdated but I agree with @cilefen - this is a question for snyk

πŸ‡¦πŸ‡ΊAustralia larowlan πŸ‡¦πŸ‡ΊπŸ.au GMT+10
πŸ‡¦πŸ‡ΊAustralia larowlan πŸ‡¦πŸ‡ΊπŸ.au GMT+10

Left a comment on the MR - is there really no way to do this from search module or similar, putting it in a specific entity class feels wrong - what about other translatable nodes?

πŸ‡¦πŸ‡ΊAustralia larowlan πŸ‡¦πŸ‡ΊπŸ.au GMT+10

Left some questions on the MR - thanks for working on this!

πŸ‡¦πŸ‡ΊAustralia larowlan πŸ‡¦πŸ‡ΊπŸ.au GMT+10

Thanks @yogesh045 - are you able to convert this to a merge request - the project only uses gitlab CI now for testing

πŸ‡¦πŸ‡ΊAustralia larowlan πŸ‡¦πŸ‡ΊπŸ.au GMT+10

It might be worth setting up a meeting to demo my work - both of you weren't in the isomorphic rendering call. It's all in my private github at the moment because I think it is generic and can be a composer package rather than part of this module.

tl;dr given a twig component, you get Javascript (not JSX) that has been transpiled to what you get from babel's react preset - so basically a function that takes props as args and uses the jsx runtime to render. It needs a lot more work but I have some tricky test cases working.

And yes, it does use the Twig token stream - but there's something important to note about Twig - it does not parse HTML - my work combines it with a mastermind/html5 and a custom event subscriber to build an AST of dom components and Twig logic. The twig token stream treats markup as plain text so there is no concept of component hierarchy. i.e it does not see a 'li' as a child of a 'ul'.

In terms of what's required here - it feels like we want comments before/after a component, comments before/after a prop and components before/after a slot.

I assume that you're going to rely on a round trip back to Drupal to update previews. My Twig to Javascript w/ jsx runtime experiment was to avoid that.

Twig doesn't really see the distinction between a prop and a slot, it just sees them as a print statement - _but_ the issue we get into here is things like logic - e.g. sometimes the twig template could be modifying a prop or passing it through a filter.

So for the comments before/after the component - we can easily do that without much work - we already do that with twig debug (and SDC debug - the emoji thing). The before/after prop/slot thing we will need to drop down to the token stream layer and try to hook into variable nodes.

πŸ‡¦πŸ‡ΊAustralia larowlan πŸ‡¦πŸ‡ΊπŸ.au GMT+10

Sounds good - we can use MSW to mock all the endpoints so it is static - at present we're only booting MSW if the NODE_ENV is not production but we could use different environment flavours to change the handlers we use. E.g. we could have a NODE_ENV=demo

πŸ‡¦πŸ‡ΊAustralia larowlan πŸ‡¦πŸ‡ΊπŸ.au GMT+10

We have a monthly DA and committers catchup where I can raise it. I don't know anyone in the promote drupal initiative.

πŸ‡¦πŸ‡ΊAustralia larowlan πŸ‡¦πŸ‡ΊπŸ.au GMT+10

In this focus on fields and SDC components we have to make sure we don't forget about blocks. Embedding listing components (eg a view) in a page is a common use case that doesn't fit the current data model. Yes I know there's a block field module, but that's an extra layer on top that we currently don't need with layout builder

πŸ‡¦πŸ‡ΊAustralia larowlan πŸ‡¦πŸ‡ΊπŸ.au GMT+10

At some point do we need to talk about formatters? Yes we might store a media reference - but we might format it using e.g. a different view mode or a responsive image format

πŸ‡¦πŸ‡ΊAustralia larowlan πŸ‡¦πŸ‡ΊπŸ.au GMT+10

Ok, let's tackle it when it becomes an issue

πŸ‡¦πŸ‡ΊAustralia larowlan πŸ‡¦πŸ‡ΊπŸ.au GMT+10

I wonder if we should speak to the promote drupal folks or the DA who're working on the D10 version of drupal.org. Whilst we need it as a temporary solution - it feels like there is long-term value in having an actual design system for drupal.org

πŸ‡¦πŸ‡ΊAustralia larowlan πŸ‡¦πŸ‡ΊπŸ.au GMT+10

Can you share the output of `composer outdated`

πŸ‡¦πŸ‡ΊAustralia larowlan πŸ‡¦πŸ‡ΊπŸ.au GMT+10

Thanks - we have some tests for that form - could we expand them to demonstrate the issue/fix?

πŸ‡¦πŸ‡ΊAustralia larowlan πŸ‡¦πŸ‡ΊπŸ.au GMT+10

Reading this am I right in thinking the comments approach is preferred over the data-attributes?

Is there a plan for how we would implement this?

The data-attributes feels more robust but also has the same brittleness as other places we've used that in core (layout builder blocks, layout builder sections, layout builder regions, contextual links, RDF).

The AST is that the work I have been doing in my spare-time on πŸ“Œ Experiment: Render Twig as React Active that I demoed in 🌱 Meeting notes: May 20th 2024 - Isomorphic rendering Fixed ?

I had paused it because it felt like it wasn't needed - but are we now saying it will be? If so I could pick it up again.

One other comment on the data-xb-text-content attribute - this sounds a lot like widgets - which is what was implemented in decoupled layout builder. This would fit well with the Component config entity because we could say 'use this widget' for this prop. We could also make the React component for each widgets swappable too (per this question from nod_)

πŸ‡¦πŸ‡ΊAustralia larowlan πŸ‡¦πŸ‡ΊπŸ.au GMT+10
πŸ‡¦πŸ‡ΊAustralia larowlan πŸ‡¦πŸ‡ΊπŸ.au GMT+10

larowlan β†’ created an issue.

πŸ‡¦πŸ‡ΊAustralia larowlan πŸ‡¦πŸ‡ΊπŸ.au GMT+10
πŸ‡¦πŸ‡ΊAustralia larowlan πŸ‡¦πŸ‡ΊπŸ.au GMT+10

larowlan β†’ created an issue.

πŸ‡¦πŸ‡ΊAustralia larowlan πŸ‡¦πŸ‡ΊπŸ.au GMT+10
πŸ‡¦πŸ‡ΊAustralia larowlan πŸ‡¦πŸ‡ΊπŸ.au GMT+10

larowlan β†’ created an issue.

πŸ‡¦πŸ‡ΊAustralia larowlan πŸ‡¦πŸ‡ΊπŸ.au GMT+10

I think we'd need to convey to the user that the tokens were it weren't available
That would likely require extra information on the plugins

πŸ‡¦πŸ‡ΊAustralia larowlan πŸ‡¦πŸ‡ΊπŸ.au GMT+10

Sorry but this assumes the recipient email is drawn from a user.
The module supports any email field, not just users.

I don't see how we can support this.

πŸ‡¦πŸ‡ΊAustralia larowlan πŸ‡¦πŸ‡ΊπŸ.au GMT+10

larowlan β†’ created an issue.

πŸ‡¦πŸ‡ΊAustralia larowlan πŸ‡¦πŸ‡ΊπŸ.au GMT+10

Unfortunately the added Cypress test doesn't seem to be running, there's no mention of layout slice in the CI output that I can see

πŸ‡¦πŸ‡ΊAustralia larowlan πŸ‡¦πŸ‡ΊπŸ.au GMT+10
πŸ‡¦πŸ‡ΊAustralia larowlan πŸ‡¦πŸ‡ΊπŸ.au GMT+10

Re #49 it won't

πŸ‡¦πŸ‡ΊAustralia larowlan πŸ‡¦πŸ‡ΊπŸ.au GMT+10

Would be neat to do per MR builds and put the build output in artifacts so we can test without local

I'll make an issue for that next week

πŸ‡¦πŸ‡ΊAustralia larowlan πŸ‡¦πŸ‡ΊπŸ.au GMT+10

No not at all
We can cherry pick the fix if need be

πŸ‡¦πŸ‡ΊAustralia larowlan πŸ‡¦πŸ‡ΊπŸ.au GMT+10

+1

πŸ‡¦πŸ‡ΊAustralia larowlan πŸ‡¦πŸ‡ΊπŸ.au GMT+10

This is a great idea!

πŸ‡¦πŸ‡ΊAustralia larowlan πŸ‡¦πŸ‡ΊπŸ.au GMT+10

Keen to get a test here - thanks

πŸ‡¦πŸ‡ΊAustralia larowlan πŸ‡¦πŸ‡ΊπŸ.au GMT+10

Getting a cypress fail here https://git.drupalcode.org/issue/experience_builder-3452828/-/jobs/1800000 but it might be a general CI failure

πŸ‡¦πŸ‡ΊAustralia larowlan πŸ‡¦πŸ‡ΊπŸ.au GMT+10

Here's some sample vitests that show unit tests for state, and some component tests - https://git.drupalcode.org/project/experience_builder/-/merge_requests/38
Here's a sample unit test for cypress - https://git.drupalcode.org/project/experience_builder/-/merge_requests/43 - unfortunately it doesn't run because we can't load components/files form outside tests/Cypress. I think we will need to merge the package.json in tests/Cypress with the one in ui in order to use it for component/unit testing. We can use it where it is for E2E.

πŸ‡¦πŸ‡ΊAustralia larowlan πŸ‡¦πŸ‡ΊπŸ.au GMT+10

Added fix and tests on top of πŸ“Œ Add tests for undo/redo Needs review

πŸ‡¦πŸ‡ΊAustralia larowlan πŸ‡¦πŸ‡ΊπŸ.au GMT+10

Rebased and pushed some undo/redo tests and test/fix for πŸ› Undo/redo - user can undo the loading of the initial state Active

πŸ‡¦πŸ‡ΊAustralia larowlan πŸ‡¦πŸ‡ΊπŸ.au GMT+10

larowlan β†’ made their first commit to this issue’s fork.

πŸ‡¦πŸ‡ΊAustralia larowlan πŸ‡¦πŸ‡ΊπŸ.au GMT+10

Copying in comms from slack thread

Some thoughts from me

  • config entity that represents a component
    • for each component the site builder can say for each prop one of the following;
      • Option 1 Fixed value (set in config entity)
      • Option 2 Dynamic value taken from base entity - mapped in config entity, site builder picks widget and formatter
      • Option 3 Arbitrary value use this widget, use this formatter, mapped in config entity *may not be needed, read on
    • In the edit form for content editors the first option above doesnt show
    • Option two (Dynamic) shows but stores in content entity field, with reference in tree
    • Option three stores arbitrary typed data in the tree *may not be needed, read on

    With this model, the config entity had a config dependency on any fields it maps to, as well as widget/formatters (module dependencies) so we can manage dependencies on fields etc

    It also has an entity-type and bundle implied by way of the fields it uses. This limits available things in the component list, can't use a component that relies on fields that aren't there
    The config entity can also record 'slots I work in' ala layout builder restrictions

    I think that also lets you change the mappings later - want the 'hero title' component to use a different field for the image, change it in the content entity, no need to update the tree

    I discussed this with @catch and he asked do we even need to distinguish between Option 2 and Option 3 - can we not just dynamically add fields to the content entity and remove option 3 - always storing values in fields on the entity.

    @catch and I brainstormed that a bit - the example I used was this

    let's say you have a tab's component, in the second tab, it uses a two-col layout, the left col has a slider in it, in that you have cards, each card has an image

    In that scenario I guess you could pick out common shapes and make fields on the fly for them - eg, in that scenario it might spot the following:

    • image field on the card => media ref
    • title field on the card, title field on the tabs, title field on columns => string field
    • body field on column two, teaser field on card => text field

    So that would be 3 fields and then the props could say 'tab 2 title is delta 3 in the title field', 'card image 3 is delta 3 in the media field'
    This would mean you need to limit components per bundle - but that is something we'd probably want anyway - LB restrictions is already per bundle. So if you said 'this component can be used on articles' we would analyse the mapping and go off and make sure the required fields exists. (note this sounds a lot like what @gabesullice is proposing in #44)

    My concern is I don't think we should be asking content-editors to 'pick the field you want to populate this prop from'. I think site-builders should make that decision ahead of time. If there's multiple options, they should make multiple components (config entities). I think asking content-editors to think about data-structures and fields is like asking them to think about formatters like LB field blocks, which is something we avoid on client projects. ***

    I think content editors should just be filling in fields using widgets the site-builder decided make sense for a prop in an SDC, no different to what they do now for LB/Content forms/Paragraphs

    With this in place the data model could store references to the config entity in place of 'type:field' in @gabesullice's example in #44

    Then if we extend this further and add a 'type' to the component config entity - we could in theory write an adapter for inline blocks (layout builder) and paragraphs. These could be like the source plugins we have on media-types. The adapters could be a plugin and we could put a single plugin collection on the component config entity. We could interact with this to trigger rendering and prop evaluation.

    *** @lauriii indicated he'd like to do user-research to confirm this

πŸ‡¦πŸ‡ΊAustralia larowlan πŸ‡¦πŸ‡ΊπŸ.au GMT+10

Thanks for that background Ben
My experience with Cypress has not been positive, I've come to associate it with brittle, slow and flaky test in client projects.
I think that has been down to how they have implemented it. I note you mention query selector and in my experience that has been the main issue. The tests have been written closely coupled to the Dom and in the end they've repeated a lot of the same mistakes people made with Behat.
I note that testing library has a set of eslint rules https://testing-library.com/docs/ecosystem-eslint-plugin-testing-library/ that explicitly deny interacting with the Dom via query selector and the like. Instead you're encouraged to write tests that make use of selectors that reflect what the user sees, eg getByRole('button', {name: 'Submit'}) and others like getByText.

I've written some unit tests for reducers on the undo/redo test issue. I'll expand that to include a component test with testing library. I'll then write a second MR that uses Cypress for the same. We then will have two to compare.

I'm not opposed to using Cypress if that's the preference, but we should try to mimic best practices and avoid proliferation of query selectors in tests.

πŸ‡¦πŸ‡ΊAustralia larowlan πŸ‡¦πŸ‡ΊπŸ.au GMT+10

Started scaffolding some stuff

Will need a rebase --onto 0.x once the undo/redo MR is merged

git rebase --onto origin/0.x 45f69ac 3452781-undo-redo-tests

πŸ‡¦πŸ‡ΊAustralia larowlan πŸ‡¦πŸ‡ΊπŸ.au GMT+10

larowlan β†’ made their first commit to this issue’s fork.

πŸ‡¦πŸ‡ΊAustralia larowlan πŸ‡¦πŸ‡ΊπŸ.au GMT+10
πŸ‡¦πŸ‡ΊAustralia larowlan πŸ‡¦πŸ‡ΊπŸ.au GMT+10

In terms of the why, a hero image is a good example - you could imagine this would be used in the full page view but also in a card view mode for a node

Having some structured data at the entity level allows for reuse

πŸ‡¦πŸ‡ΊAustralia larowlan πŸ‡¦πŸ‡ΊπŸ.au GMT+10

@catch it's not fields, it's references in the tree to values in other fields

Eg a hero title component might reference a hero image field and title field in a node. At present if the hero image field is deleted it fatals

The intent is that some components store arbitrary data but some are backed by structured fields. The intention is this will allow editing the field values and the 'layout' in one place

πŸ‡¦πŸ‡ΊAustralia larowlan πŸ‡¦πŸ‡ΊπŸ.au GMT+10
πŸ‡¦πŸ‡ΊAustralia larowlan πŸ‡¦πŸ‡ΊπŸ.au GMT+10
πŸ‡¦πŸ‡ΊAustralia larowlan πŸ‡¦πŸ‡ΊπŸ.au GMT+10
πŸ‡¦πŸ‡ΊAustralia larowlan πŸ‡¦πŸ‡ΊπŸ.au GMT+10
πŸ‡¦πŸ‡ΊAustralia larowlan πŸ‡¦πŸ‡ΊπŸ.au GMT+10

😍

πŸ‡¦πŸ‡ΊAustralia larowlan πŸ‡¦πŸ‡ΊπŸ.au GMT+10

Yep, copy paste error

πŸ‡¦πŸ‡ΊAustralia larowlan πŸ‡¦πŸ‡ΊπŸ.au GMT+10

larowlan β†’ made their first commit to this issue’s fork.

πŸ‡¦πŸ‡ΊAustralia larowlan πŸ‡¦πŸ‡ΊπŸ.au GMT+10

larowlan β†’ made their first commit to this issue’s fork.

πŸ‡¦πŸ‡ΊAustralia larowlan πŸ‡¦πŸ‡ΊπŸ.au GMT+10

larowlan β†’ created an issue.

πŸ‡¦πŸ‡ΊAustralia larowlan πŸ‡¦πŸ‡ΊπŸ.au GMT+10

larowlan β†’ made their first commit to this issue’s fork.

πŸ‡¦πŸ‡ΊAustralia larowlan πŸ‡¦πŸ‡ΊπŸ.au GMT+10
πŸ‡¦πŸ‡ΊAustralia larowlan πŸ‡¦πŸ‡ΊπŸ.au GMT+10

We need to update tests to account for πŸ› User logout is vulnerable to CSRF Fixed

Production build 0.69.0 2024