Problem/Motivation

Some islands have a related Form class in src/Form to process submissions:

 $ ls src/Form/ | sort
BlockStylesForm.php >> UiStylesPanel
CssVariablesForm.php >> UiSkinsPanel
DisplayBuilderDeleteForm.php
DisplayBuilderForm.php
SlotSourceForm.php >> InstanceFormPanel
VisibilityConditionForm.php >> VisibilityConditionPanel

That's confusing and may be the mark of an incomplete island API.

Proposed resolution

Move all islands logic to the islands plugin classes.

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:

Comments

pdureau created an issue. See original summary.

goz’s picture

Assigned: Unassigned » goz

pdureau’s picture

Status: Active » Needs work

Thanks you goz. That looks exciting. Also, that's great you are commenting your own MR: https://git.drupalcode.org/project/display_builder/-/merge_requests/13

However:

  • Why did you add IslandPluginFormTrait to ResetButton?
  • Instead of adding $current_island_id on onUpdate() method, is there an other way of passing the data? Ideally, this MR will only alter the 4 island plugins with forms.
  • Can you fix the pipeline feedbacks first? https://git.drupalcode.org/project/display_builder/-/merge_requests/13/p... (only the stuff related to your MR)
goz’s picture

Assigned: goz » Unassigned
Status: Needs work » Needs review

Why did you add IslandPluginFormTrait to ResetButton?

You are right, i miss this one.
It's a plugin, i should add plugin form logic here to.

Instead of adding $current_island_id on onUpdate() method, is there an other way of passing the data? Ideally, this MR will only alter the 4 island plugins with forms.

Unfortunatelly not, or may be using another static service...

goz’s picture

Everything is green except css lint. Yipee !

I take a look on ResetButton and we are good

goz’s picture

MR can be reviewed

goz’s picture

Issue tags: +BarCampPerpignan2025
pdureau’s picture

Assigned: Unassigned » mogtofu33

The warning in the pipeline is not about this work. So ok for review.

mogtofu33’s picture

Title: [1.0.0-beta1] Remove Island's specific Form classes » Remove Island's specific Form classes
Issue tags: +display_builder-1.0.0-alpha1
goz’s picture

Status: Needs review » Needs work
goz’s picture

Status: Needs work » Needs review
mogtofu33’s picture

Assigned: mogtofu33 » Unassigned
Status: Needs review » Needs work
Issue tags: -display_builder-1.0.0-alpha1 +display_builder-1.0.0-alpha2

Currently it seems the form lost it's state, at least for UI Styles and UI Tokens.

From an empty display builder with ui suite bootstrap:

  • Add a button
  • Add a token inside, fill a text
  • Apply a style and a token to the button and the token (font h2, border color and background color for example)
  • They are applied properly, but not kept in the instance form
  • Refresh the page
  • They are applied properly, but not kept in the instance form

Expected is to have the style and token selected in the instance form with a message (current behavior on 1.0.x).

goz’s picture

Status: Needs work » Needs review
mogtofu33’s picture

Status: Needs review » Needs work

Still have the issue.
In current BlockStylesForm::buildForm, the $data['styles'] contains the selected value as:

[
  "styles" => [
    "selected" => [
      "typography" => "display-1"
    ]
  ]
]

On this branch, the code is now in UiStylesPanel::buildForm as $this->data['styles']. Problem is it's nested in:

  "_third_party_settings" => [
    "ui_styles" => [
      "styles" => [
        "selected" =>  [
          "typography" => "display-1"
        ]
      ]
    ]
  ]
]

So it does not apply.

goz’s picture

It's strange, in the last commit, i take care of taking value from _third_party_settings.

In IslandPluginBase::build() :

    // First, get specific data for the plugin.
    if (isset($data['_third_party_settings'][$this->getPluginId()])) {
      $this->data = $data['_third_party_settings'][$this->getPluginId()];
    }
goz’s picture

I was reproducing #13, i don't reproduce it anymore.
Are you sure you are up to date with the MR ?
Or may be you make another test than #13 which fails ?

mogtofu33’s picture

My bad it's working better. The switch between the 2 branches made a false positive.
But I have an other problem that I thought was related but not, when loading a fixture like bootstrap demo it has warning when rendering for each element:

Warning: Uninitialized string offset 0 in Drupal\Core\Render\Element::property() (line 29 of /var/www/html/web/core/lib/Drupal/Core/Render/Element.php)

But this code do not touch the rendering, so I guess there is a change somewhere in the data of the builder saved, will investigate.

Moving for now to alpha2 to have time to finalize.

goz’s picture

I don't reproduce, in an instance without this MR, nor in instance with this MR.
I hadn't loading fixtures before, may be you were on an existed fixture ?

pdureau’s picture

Assigned: Unassigned » mogtofu33
Status: Needs work » Needs review
pdureau’s picture

Assigned: mogtofu33 » pdureau

I will also have a look

pdureau’s picture

Assigned: pdureau » Unassigned
Status: Needs review » Needs work

when loading a fixture like bootstrap demo it has warning when rendering for each element:
Warning: Uninitialized string offset 0 in Drupal\Core\Render\Element::property() (line 29 of /var/www/html/web/core/lib/Drupal/Core/Render/Element.php)

From a fresh install, on commit d6a3f04d46a21c15e66a8b62fe25b8a9e3c98ebe (HEAD -> 3529103-1.0.0-beta1-remove-islands, display_builder-3529103/3529103-1.0.0-beta1-remove-islands):

  1. I install & set default ui_suite_bootstrap from /admin/appearance
  2. I go to /admin/structure/display-builder/instance/add
  3. I create a demo from [display_builder_devel] Ui suite bootstrap demo
  4. I also have Warning: Uninitialized string offset 0 in Drupal\Core\Render\Element::property() (line 26 of core/lib/Drupal/Core/Render/Element.php).
goz’s picture

My bad, digging for the error, it seems i previously met this, but certainly fix it in ui_patterns or elsewhere which is not in display_buider... and forgot about it.

The following error

     Warning: Uninitialized string offset 0 in Drupal\Core\Render\Element::property() (line 29 of /var/www/html/web/core/lib/Drupal/Core/Render/Element.php)

comes from a radio element with an option with empty key :

{
  "#type": "radios",
  "#title": "Typography",
  "#options": {
    "": "- None -",
    "h1": "<span class=\"ui-styles-source-select-plugin-option h1\">\u0391\u03b1<\/span>Heading 1",
(...)
  },
  (...)
  "#id": "edit-styles-wrapper-text-ui-styles-typography",
  "#name": "styles[wrapper][text][ui_styles_typography]",
  "#value": "",
  "": {
    "#type": "radio",
    "#title": "- None -",
    "#return_value": "",
    "#default_value": "",
    (...)
    "#name": "styles[wrapper][text][ui_styles_typography]",
    "#value": "",
    "#ajax_processed": false,
    "#sorted": true,
    "#isDisplayBuilder": true
  },
  (...)
  "#isDisplayBuilder": true
}

Strangely, i have this issue only for this MR, not from 1.0.x and in 1.0.x, this radios element is not loaded (so no error). BUT the radios element is not displayed, so i guess the MR process something which should not

goz’s picture

Difference between 2 versions :

In 1.0.x, UiStylesPanel build form is called only when island is panel island is displayed when clicking on the icon.
In MR, UiStylesPanel build form is called when page is loaded

So MR change more things that expected

goz’s picture

Status: Needs work » Needs review

I rebased my branch to not get other commits.

This issue was very hard, hope there will be no more errors.
Fingers crossed.

pdureau’s picture

Assigned: Unassigned » mogtofu33
mogtofu33’s picture

Assigned: mogtofu33 » Unassigned
Status: Needs review » Needs work

Still have the issue, as soon as UI Styles island is enabled, I have multiple warning.

goz’s picture

I have no warnings :/

Can you give me more informations to reproduce ?

Which theme do you use, do you start from a fixture, on which component ?
May be a screencast will help

Thanks

pdureau’s picture

Assigned: Unassigned » pdureau
Status: Needs work » Needs review

I will review too. Team work :)

pdureau’s picture

Assigned: pdureau » Unassigned
Status: Needs review » Needs work

From a fresh install, on commit c2d2277ce764be85497a01810aa6e991996f3a00:

  1. I install & set default ui_suite_bootstrap from /admin/appearance
  2. I go to /admin/structure/display-builder/instance/add
  3. I create a demo from [display_builder_devel] Ui suite bootstrap demo
  4. I still have Warning: Uninitialized string offset 0 in Drupal\Core\Render\Element::property() (line 26 of core/lib/Drupal/Core/Render/Element.php). just by loading the builder

Still have the issue, as soon as UI Styles island is enabled, I have multiple warning.

If I disable UI Styles panel in /admin/structure/display-builder/default, no more warning when I load the builder.

I i enable back UI Styles panel in /admin/structure/display-builder/default, warnings are back.

goz’s picture

Status: Needs work » Needs review

Sorry for the false positive, i forgot to remove some tests in core which make my instance not relevant.
I spend a big part of my weekend on it... and burn too much neurons.

Finally, took less time this afternoon to fix it once i reproduce again.

pdureau’s picture

Assigned: Unassigned » mogtofu33

  • mogtofu33 committed 7b31071a on 1.0.x authored by goz
    Issue #3529103 by goz, mogtofu33, pdureau: Remove Island's specific Form...
mogtofu33’s picture

Assigned: mogtofu33 » Unassigned
Status: Needs review » Fixed

Thanks for the hard work, let's go with that!

  • mogtofu33 committed cfa6bee5 on 1.0.x
    Revert "Issue #3529103 by goz, mogtofu33, pdureau: Remove Island's...

Status: Fixed » Closed (fixed)

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