[2.0.0-beta1] Sources don't support required - #title missing

Is it a beta1 issue?

Understood now. I will share my thoughts.

Don't implement "required" property

SDC is supporting "required" for both slots and props:

name: Tabs
props:
  type: object
  required:
    - foo
  properties:
    foo:
      title: foo
      type: string
slots:
  body:
    title: Body
    required: true

In my humble opinion, this is not a feature but an issue I was very vocal against. Required data is best fitting with business modelling, content management and data storage. UI artefacts, including components, must be the more loose possible. It is better to rely on default values.

Also, it is very hard to enforce required data. With the form builder, it is possible, by checking the filled data. But only for "widget" sources (textfield, URL, number...), not for "data from Drupal" sources (menu, field property...).

Moreover,

Or, maybe, only for props, sometimes

However, UI Patterns Settings 2.x is also proposing such a feature:

  settings:
    textfield:
      type: textfield
      label: Textfield
      preview: text preview
      required: true

And we want to cover the full scope of UI Patterns Settings (not all the setting type, but all the mechanisms).

So, let's implement required for props, and props only, for some widgets. While keeping a warning in ui_patterns_devel's Component validator telling using a default() Twig filter would be better.

Implementation proposal

On PropTypePluginBase::getSummary(), add a message telling the prop is mandatory (only for props). Careful, the information is outside the prop definition.

On source widgets (only for props), add the Form API #required property (default value: FALSE, of course):

• AttributesWidget?
• CheckboxesWidget ?
• CheckboxWidget
• ListTextareaWidget
• NumberWidget
• SelectWidget
• TextfieldWidget

Don't forget to add the migration of this property in a href="https://git.drupalcode.org/project/ui_patterns/-/blob/2.0.x/modules/ui_p...">ComponentConverter::getPropsFromSettings()

So...

OK with that?

I will give a try

Only legacy migration left

My findings:

* We have double titles now if the field is required.

To solve this I think we have two options.

• Either all sources have an title. Complex sources provides there own fieldset with the title of the attirbute
• We hide the title with css (quite ugly)
• Overwrite the empty error message.

I would prefer 1 because it is straight forward.

Right now the source must call addRequired. Maybe there is a better way to implement that.

• Move settingsForm to the base plugin class. Inside this function settingsFormSource is called which is implemented by the sources. settingsForm handles all "common" stuff like set title and set required.
• We do something like form elements doing. getInfo and this returns addRequired.
• Or just renaming: addRequired to handleRequireForm

No hard opinion about that.

We should move this to beta2.

@just_like_good_vibes what do you think?

Thanks for your revizw. We keep it in beta1

Hello,
globally that sounds ok for me.
A little remark about the implementation : the method name "addRequired" is not totally helpful in my opinion. handleRequiredProp or something like that ? and why not passing by reference the form object instead of returing it?

Also, we have like an UI issue about the title. When it is required, we add a title if not present to the form element. but indeed that could produce a duplicate title, because typically the source form is already in a propform or slotform where a title is already provided.
why not putting "Required" as a title when no title is found?

When it is required, we add a title if not present to the form element.

Indeed.

why not putting "Required" as a title when no title is found?

I was thinking about that too, but I am not comfortable about "hijacking" the title and I am wandering about potential accessibility issues.

About Christian's proposals:

• Either all sources have an title. Complex sources provides there own fieldset with the title of the attirbute
• We hide the title with css (quite ugly)
• Overwrite the empty error message.

I find the first one interesting but the fieldset also include the prop selector, so it is always expected to be there.

Maybe, we can keep the "required" management in the source form,a s already done in the MR, but move the "visual clue" to the fieldset management in the Form builder, I will do a proposal

What do you think about this proposal: https://git.drupalcode.org/project/ui_patterns/-/merge_requests/164/diff... ?

i checked it. ok let's merge ?

rebased and completed thanks to Christian remarks