Paris
Account created on 13 April 2012, over 13 years ago
  • Engagement Manager at Smile 
  • Business Unit Manager at Linagora 
#

Merge Requests

More

Recent comments

🇫🇷France pdureau Paris
🇫🇷France pdureau Paris
🇫🇷France pdureau Paris

Thanks a lot to everybody for this hard work ❤️

I have rebased   [PP-1] Allow schema references in Single Directory Component prop schemas Postponed  and everything is working perfectly

🇫🇷France pdureau Paris

Rebase is done.

The MR is full green and only 30-40 line of PHP code added to Core (blank lines, comments and tests removed).

🇫🇷France pdureau Paris

Add stream wrappers to access extension files Needs work is merged, I will rebase and send back to review.

🇫🇷France pdureau Paris

Thanks nod_

🇫🇷France pdureau Paris

Mikael is taking the related UIP2 issue Slots restrictions according to suggestions and cardinality Active so it is the best candidate for this task

🇫🇷France pdureau Paris

Hello,

I am testing with an existing content display override in Display Builder.

The updb was run successfully:

 $ ddev drush updb
  ui_patterns_field   10001       hook_update_n   10001 - Add additional columns. Add third_party_settings and node_id to ui_patterns_source fields.  

However, when I click on "Publish" button in the content display override in Display Builder, I get this:

Drupal\Core\Entity\EntityStorageException: SQLSTATE[42S22]: Column not found: 1054 Unknown column 'field_teaser_third_party_settings' in 'INSERT INTO': INSERT INTO "node__field_teaser" ("entity_id", "revision_id", "bundle", "delta", "langcode", "field_teaser_source_id", "field_teaser_source", "field_teaser_third_party_settings", "field_teaser_node_id") VALUES (...) in Drupal\Core\Entity\Sql\SqlContentEntityStorage->save() (line 815 of core/lib/Drupal/Core/Entity/Sql/SqlContentEntityStorage.php).
🇫🇷France pdureau Paris

Very exciting but too ambitious for beta2.

🇫🇷France pdureau Paris

I would prefer a way for users to add support for additional units rather than hard coding to what is in the spec.

Let's play safe, avoid any drupalism, and follow the upstream specs for the 11.3 MR.

However, thanks to the plugin system, Drupal projects may be able override the plugin, adding their own units:

use Drupal\Core\Theme\Plugin\DesignTokenValue\Dimension as CoreDimension;

#[DesignTokenValue(
  id: 'dimension',
  label: new TranslatableMarkup('Dimension'),
)]
class Dimension extends CoreDimension {

  public const array UNIT_TYPES = [
    'px',
    'rem',
   'whatever'
  ];
🇫🇷France pdureau Paris

We may need a ComponentSource plugin type, like Canvas does, with 3 plugins:

  • SDC, as we already have
  • Layout plugins for the current feature
  • Theme page, where every region is a slot and the template is page.html.twig, to address Block layout import Active
🇫🇷France pdureau Paris

Update 3 months after.

We have 4 situation to checks when I embed a display (teaser) in the other (full/default) with the ui_patterns_field's: Render Source formatter:

  • None of them are overridden: ❌ The teaser is not visible, which may be normal because nothing is stored in the field
  • Only Teaser is overridden: ✅ the teaser is embedded
  • Only Default is overridden: ❌ The teaser is not visible, which may be normal because nothing is stored in the field
  • Both are overridden: ✅ the teaser is

What is happening when we put the formatter of a display in the same display ? Infinite recursion?

Yes, infinite recursion & memory limit.

Is it a way of implementing lock mechanism like layout_builder_lock is providing?

We have a dedicated issue now: #3551232: Add a lock system

So, 2 remaining tasks:

  • Do we display the entity view display config when the field is empty?
  • Prevent infinite recursion when a field overriden a display is rendered in the same display.
🇫🇷France pdureau Paris

Let's talk about that in early beta2 phase

🇫🇷France pdureau Paris

HI Jean, we may need your help. It is hard to guess what is causing the playwright fail in await element.click({ position: { x: 5, y: 10 } })

🇫🇷France pdureau Paris

Do we also need to activate js.preprocess for playwright?

🇫🇷France pdureau Paris

i can help a bit by proposing a MR

🇫🇷France pdureau Paris

All green :)

🇫🇷France pdureau Paris

I have added the 2 test cases I was able to add.

Only line 190 & 199 may not be covered, but another mechanism is preventing buildSingleBlock() to be reached (which is is good thing in my opinion)

🇫🇷France pdureau Paris

works good for me

🇫🇷France pdureau Paris

thanks @juc1. I will have a look

🇫🇷France pdureau Paris

We may need a ComponentSource plugin type, like Canvas does, with 3 plugins:

  • SDC, as we already have
  • Layout plugins for [1.0.0-beta2] Layout plugins support Active
  • Theme page, where every region is a slot and the template is page.html.twig, to address this feature
🇫🇷France pdureau Paris

Important to deal with that ASAP

🇫🇷France pdureau Paris

If OK today, let's move this to beta1. If not, let's put it back to beta2.

🇫🇷France pdureau Paris

Hi @juc1, thanks for the work, can you:

  • fix pipelines errors (mostly phpcs)?
  • move group logic to its own protected property in PresetLibraryPanel?

The prefill of group value with the SDC group is unexpected but looks like a good idea :) Maybe we can do the same with other sources ?

  • if Drupal block plugin, the block category
  • for other slot sources, just the source label

Then, users will be able to edit the group in admin UI anyway.

🇫🇷France pdureau Paris

Let's move this issue to beta2 as a follow-up of 📌 Follow Core's HTMX integration Active and let's start by:

  • adopting the Htmx helper from Core
  • Remove unused HtmxTrait::addTarget()
  • Move ::setTrigger() to HtmxEvents
🇫🇷France pdureau Paris

I move to beta2 to explore the declarative shadow dom idea following JohnAlbin talk :https://www.youtube.com/watch?v=04YKlSNf89o

Let's be careful about JS events in and out the sandbox.

If it works, we will have 2 proposals:

  • we keep display builder in front theme and we display contextual panels with admin theme
  • we move display builder to admin theme and we display builder abd preview panel with front theme
🇫🇷France pdureau Paris

pdureau created an issue.

🇫🇷France pdureau Paris

[Entity] ➜ [Field] source is not directly exposed anymore and we can directly drop fields in the builder.

However, this source is still the unit of configuration we are manipualting in the contextual panel because we can switch to another field in the first select input.

I see some quirks when JS preprocess is activated. For example:

  • the contextual panel title don't change when we switch the field
  • most of the config changes (pick a formatter, pick a property...) don't update and we need to click on "Update" button (see 📌 Remove Update button from ContextualFormPanel Active )

But I have the same when JS preprocess is disabled. So, it seems we can remove the "Disable JavaScript files aggregation" advice.

🇫🇷France pdureau Paris

"Update" button is still useful with the "entity -> field" source.

🇫🇷France pdureau Paris

Let's move to beta2 if not merged today.

🇫🇷France pdureau Paris

Once Jean has shared the cover HTML file, let's have a look:

  • if doable today, lets keep in beta1
  • if not, let"s move to beta2
🇫🇷France pdureau Paris

careful, i have rebased the MR.

It works OK for me.

Next steps:

  • fix pipeline KOs
  • check if components or blocks with a specific asset library, when added for the first time in BuilderPanel, have the expected CSS and/or JS in the HTMX resposne
  • use the Htmx helper from Core? Pierre has a branch somewhere with a conversion. But it can be a follow-up issue.
🇫🇷France pdureau Paris

I am still not reproducing while using display_builder_theme_test with Firefox outside of Playwright:

Investigation still ongoing.

🇫🇷France pdureau Paris

Sure

🇫🇷France pdureau Paris

done

🇫🇷France pdureau Paris

I can do a bit better :)

🇫🇷France pdureau Paris

pdureau created an issue.

🇫🇷France pdureau Paris

pdureau made their first commit to this issue’s fork.

🇫🇷France pdureau Paris

pdureau created an issue. See original summary .

🇫🇷France pdureau Paris

pdureau created an issue.

🇫🇷France pdureau Paris

An other point is you assumed the 'profile' key in the tests, I changed it for the const ConfigFormBuilderInterface::PROFILE_PROPERTY.

oh! sure! 👍

You left some empty third arguments in the self::assert methods, it's used to describe the test, it makes it easier to read, I personally don't use it enough but that's good practice.

Good to know.

🇫🇷France pdureau Paris

I will create a dedicated issue for beta2

🇫🇷France pdureau Paris

For the split and data provider let me know if you want me to do it.

That would be nice if you do it because I am not sure about the way to do it:

  • split: the 2 access handlers are working together, checking 2 layers of the same access, so it seems odd to me to split them (it may be because I am new to kernel testing)
  • data provider: i don't see data providers in the other kernel tests we have, I was beliveing it was more a unit test thing
🇫🇷France pdureau Paris

Don't worry. I will rebase your stuff and keep working ConfigFormBuilderTest

🇫🇷France pdureau Paris

Need to fix test ConfigFormBuilderTest, split AccessControlTest for each entity types and use data provider to have lighter and easier to maintain tests.

I would be happy to do this and fix the phpunit fail probably introduced by the rebase: https://git.drupalcode.org/project/display_builder/-/jobs/7264278

🇫🇷France pdureau Paris

Curret work merged in beta1. We keep the ticket for follow-up.

🇫🇷France pdureau Paris

started

🇫🇷France pdureau Paris

It seems the issue is fixed by 📌 Follow Core's HTMX integration Active :)

🇫🇷France pdureau Paris

Jean, on your 4 comments:

  • I have fixed 2 of them
  • I have answered the 2 others
🇫🇷France pdureau Paris

I will try to do it

🇫🇷France pdureau Paris

pdureau created an issue.

🇫🇷France pdureau Paris

We need this Core patch : 🐛 PageDisplayVariant does not transmit cache metadata Active

🇫🇷France pdureau Paris

PageDisplayVariantSelectionEvent implements RefinableCacheableDependencyInterface.

So let's explore this

  • we add a custom cache tag in PageDisplayVariantSelectionEvent
  • we keep ::isImpactingPageVariantDetection()
  • instead of what flushing the cache in PageLayout::postSave(), we invalidate the tag
🇫🇷France pdureau Paris

Finally, I did more than expected:

in src/Entity/Instance.php and src/ProfileViewBuilder.php I am proposing a fix of the initial scope.

in modules/display_builder_page_layout/src/Entity/PageLayout.php and modules/display_builder_page_layout/src/Plugin/DisplayVariant/DisplayBuilderPageVariant.php I am proposing a fix for page variants maangement.

🇫🇷France pdureau Paris

Let's not forget to update install.md

🇫🇷France pdureau Paris

Functional review: OK for me.

What about the technical implementation? Is it OK too?

🇫🇷France pdureau Paris

It seems to work well everywhere... but in /admin/structure/page-layout/{page_layout}/builder where I get:
Warning: Undefined array key "#display" in Drupal\Core\Render\Element\StatusMessages::generatePlaceholder() (line 54 of core/lib/Drupal/Core/Render/Element/StatusMessages.php).

Production build 0.71.5 2024