Problem/Motivation

The "TipPluginText" Tip plugin has several "base" form elements which should be part of the "TipPluginBase". See https://git.drupalcode.org/project/tour/-/blob/2.0.x/src/Plugin/tour/tip...

Steps to reproduce

Proposed resolution

Move "label", "id", "plugin", "weight", "selector" and "position" form elements to TipPluginBase.

Remaining tasks

User interface changes

API changes

Data model changes

Issue fork tour-3548868

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

grevil created an issue. See original summary.

grevil’s picture

grevil commented

@smustgrave, if you agree, I can fire up a quick MR.

grevil’s picture

grevil commented

This is also reflected in the already existing schema:

tour.tip:
  type: mapping
  label: 'Tour tip'
  mapping:
    id:
      type: machine_name
      label: 'ID'
      constraints:
        # Tour IDs also allow dashes.
        Regex:
          pattern: '/^[a-z0-9_-]+$/'
          message: "The %value machine name is not valid."
    plugin:
      type: string
      label: 'Plugin'
      constraints:
        PluginExists:
          manager: plugin.manager.tour.tip
          interface: '\Drupal\tour\TipPluginInterface'
    label:
      type: required_label
      label: 'Label'
    weight:
      type: weight
      label: 'Weight'
    position:
      type: string
      label: 'Position'
      constraints:
        Choice:
          - auto
          - auto-start
          - auto-end
          - top
          - top-start
          - top-end
          - bottom
          - bottom-start
          - bottom-end
          - right
          - right-start
          - right-end
          - left
          - left-start
          - left-end
    selector:
      type: string
      label: 'Selector'
      nullable: true
      constraints:
        NotBlank:
          allowNull: true

tour.tip.text:
  type: tour.tip
  label: 'Textual tour tip'
  mapping:
    body:
      type: text
      label: 'Body'
anybody’s picture

anybody
Primary language German
Location Porta Westfalica
commented

Thanks @grevil yes totally makes sense architecturally, also to allow clean, unified plugins in contrib without reinventing the wheel or copying common code.
I think it just wasn't needed in the past, because there is only one plugin. But clear to see these things are very general :)

smustgrave’s picture

smustgrave commented

If you can do it and doesn't break anything go for it.

grevil’s picture

grevil commented
Status: Active » Needs review

Done! Please review!

grevil opened merge request !118

anybody’s picture

anybody
Primary language German
Location Porta Westfalica
commented

Thanks @grevil! I'm 50/50...

I'm 100% sure that ID, title, etc. should go into the base class!

I'm a bit (50/50) unsure about:

  • body
  • selector
  • position

because they might be required for 90% of all Tip Plugins and 95% of all "content" Tip Plugins.

Your point with the schema on the other hand is totally valid, looking at that, I'd definitely vote to unify all into the Base.
An alternative would be to put those three into a trait, sth. like src/TipPluginContentTrait.php but for the schema that would still mean, that these three should be moved out of the general tip and into tour.tip.text.

What do you think @smustgrave?

I might tend to over-optimization here...?

smustgrave’s picture

smustgrave commented
Status: Needs review » Needs work

Maybe we should follow what the schema has where body is attached to the text plugin. Thinking if plugins get created for images or something other then text.

grevil’s picture

grevil commented
Status: Needs work » Needs review

Maybe we should follow what the schema has where body is attached to the text plugin

The MR should have reflected that already, but it didn't. Now it should be correct.

  • smustgrave committed b1a32575 on 2.0.x authored by grevil 
    [#3548868] fix: TipPluginText: "label", "id", "plugin", "weight", "...
smustgrave’s picture

smustgrave commented
Status: Needs review » Fixed

Thanks LGTM!

Now that this issue is closed, please review the contribution record.

As a contributor, attribute any organization that helped you, or if you volunteered your own time.

Maintainers, please credit people who helped resolve this issue.

grevil’s picture

grevil commented

Thank you, @smustgrave! :)

Could you tag a new release? Would appreciate it!

smustgrave’s picture

smustgrave commented

Waiting on 1 more ticket but should this week

Status: Fixed » Closed (fixed)

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