- Issue created by @Christian.wiedemann
- Assigned to Christian.wiedemann
- Assigned to pdureau
- Assigned to Christian.wiedemann
- Status changed to Postponed: needs info
about 2 months ago 7:36pm 22 July 2024 - 🇫🇷France pdureau Paris
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?
- Status changed to Active
about 1 month ago 8:15am 1 August 2024 - Assigned to pdureau
- Status changed to Needs work
about 1 month ago 4:05pm 1 August 2024 - Assigned to Christian.wiedemann
- Status changed to Needs review
about 1 month ago 11:02pm 2 August 2024 - 🇩🇪Germany Christian.wiedemann
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.
- Status changed to Needs work
about 1 month ago 9:43am 4 August 2024 - Assigned to just_like_good_vibes
- Status changed to Needs review
about 1 month ago 9:45am 4 August 2024 - 🇫🇷France just_like_good_vibes PARIS
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? - Assigned to Christian.wiedemann
- Assigned to pdureau
- Status changed to Needs work
about 1 month ago 6:55am 5 August 2024 - 🇫🇷France pdureau Paris
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
- Assigned to Christian.wiedemann
- Status changed to Needs review
about 1 month ago 8:09am 5 August 2024 - 🇫🇷France pdureau Paris
What do you think about this proposal: https://git.drupalcode.org/project/ui_patterns/-/merge_requests/164/diff... ?
- Status changed to RTBC
about 1 month ago 3:15pm 6 August 2024 - Status changed to Needs review
about 1 month ago 12:01pm 8 August 2024 - 🇫🇷France just_like_good_vibes PARIS
rebased and completed thanks to Christian remarks
-
just_like_good_vibes →
committed 7fe372c7 on 2.0.x authored by
pdureau →
Issue #3461277 by pdureau, Christian.wiedemann: Sources don't support...
-
just_like_good_vibes →
committed 7fe372c7 on 2.0.x authored by
pdureau →
- Issue was unassigned.
- Status changed to Fixed
about 1 month ago 12:05pm 8 August 2024 - Status changed to Fixed
26 days ago 2:47pm 12 August 2024