Problem/Motivation

Should be done after #3529103: Remove Island's specific Form classes

The islands are not configurable in /admin/structure/display-builder/{id}

Proposed resolution

Do we need to implement Drupal\Core\Plugin\PluginFormInterface on islands plugins?

Let's start using this interface by adding those configs:

  • components providers in component library (by default, disable display_builder). Today, it is hardcoded in ::getDefinitionsForProvider()
  • block providers in block library (by default,disable ui_patterns_layouts)
  • display "Clear" button in history buttons (must be hidden by default)
  • buttons with labels, icons, or both.

Issue fork display_builder-3529067

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:

About issue forks

Comments

pdureau created an issue. See original summary.

pdureau’s picture

pdureau commented
Title: [1.0.0-alpha1] Make Island plugins configurable » [1.0.0-alpha2] Make Island plugins configurable
pdureau’s picture

pdureau commented
Issue summary: View changes
pdureau’s picture

pdureau commented
Issue summary: View changes
mogtofu33’s picture

mogtofu33 commented
Issue tags: +display_builder-1.0.0-alpha2
pdureau’s picture

pdureau commented
Issue summary: View changes
mogtofu33’s picture

mogtofu33 commented
Title: [1.0.0-alpha2] Make Island plugins configurable » Make Island plugins configurable
Issue summary: View changes
Issue tags: -display_builder-1.0.0-alpha2 +display_builder-1.0.0-alpha3
Related issues: +#3529103: Remove Island's specific Form classes
pdureau’s picture

pdureau commented

It would be better to start working on this after #3529103: Remove Island's specific Form classes is merged

christian.wiedemann made their first commit to this issue’s fork.

christian.wiedemann’s picture

christian.wiedemann commented
Assigned: Unassigned » christian.wiedemann

christian.wiedemann opened merge request !33

christian.wiedemann’s picture

christian.wiedemann commented

Hi all, I set this to review. It is not done but a short review would help me to know if I am on the right path. We can discuss that on wendsday

I did:

* Add PluginFormInterface, ConfigurableInterface to IslandInterface
* Implement a sample base form with a dummy label.
* Add forms.
* Pass configuration to island.

Questions:

* Should we make PluginFormInterface optional and add a basic implementation as a trait?
* Are there some basic configurations for every island.

christian.wiedemann’s picture

christian.wiedemann commented
Assigned: christian.wiedemann » Unassigned
Status: Active » Needs review
pdureau’s picture

pdureau commented
Assigned: Unassigned » mogtofu33
Issue summary: View changes

Are there some basic configurations for every island.

it is debatable, but i don't see any configuration shared by all islands. I can picture islands with no configuration.

By the way, you didn't add those first configs:

  • components providers in component library (by default, disable display_builder). Today, it is hardcoded in ::getDefinitionsForProvider()
  • block providers in block library (by default, disable ui_patterns_layouts)
  • display "Clear" button in history buttons (must be hidden by default)
  • buttons with labels, icons, or both.

Do we focus this issue to the mechanism, and move the addition of the first configs to island plugins in a follow-up issue?

pdureau’s picture

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

TODO in the scope of this issue:

  • display "Clear" button in history buttons (must be hidden by default)

Pierre will move this to another issue:

  • components providers in component library (by default, disable display_builder). Today, it is hardcoded in ::getDefinitionsForProvider()
  • block providers in block library (by default,disable ui_patterns_layouts)
christian.wiedemann’s picture

christian.wiedemann commented
Status: Needs work » Needs review

I finalized the island configuration part. Happy about review.

pdureau’s picture

pdureau commented
Status: Needs review » Needs work

The expected config was not to display all the buttons or not, but only the "Clear" button:

display "Clear" button in history buttons (must be hidden by default)

pdureau’s picture

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

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

For the the configuration form of History island raise an ajax error without message.

  • Fresh install
  • Go to Default islands configuration
  • Click configure in front of History
  • -> Ajax error
mogtofu33’s picture

mogtofu33 commented

I struggled on the rebase so canceled it, would be probable easier to squash the current mr to one commit.

mogtofu33 changed the visibility of the branch 3529067-make-island-plugins to hidden.

mogtofu33 opened merge request !41

mogtofu33’s picture

mogtofu33 commented

I created a rebased version, bug is still there, and cannot add a new DB config as well.

mogtofu33’s picture

mogtofu33 commented
Assigned: Unassigned » mogtofu33

Fixed the configure click problem, the route {display_builder}/{instance_id} was conflicting with {display_builder}/delete
Still have a problem when creating a new config of DB islands, the route require an id which is not ready yet and will raise an error.

mogtofu33’s picture

mogtofu33 commented

Fixed the new case, some style review and should be ok.

  • mogtofu33 committed a6891e6f on 1.0.x 
    Issue #3529067 by christian.wiedemann, mogtofu33, pdureau: Make Island...
mogtofu33’s picture

mogtofu33 commented
Assigned: mogtofu33 » Unassigned
Status: Needs work » Fixed

Status: Fixed » Closed (fixed)

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

mogtofu33 closed merge request !33