Add a widget setting to select fieldset or container as the address wrapper

Thanks everyone, this is almost ready. I gave it a whirl on a test install and it works great.

Here are the last points worth discussing:

1) We are calling the setting "render" and labeling it "Render element". Are we convinced this is the most intuitive naming? Views and Field Group tend to talk about "wrappers" and "wrapper elements" instead. Perhaps it would make more sense to rename "render" to "wrapper" and have the label be one of: "Wrapper"/"Wrapper element"/"Wrapper type"? Thoughts welcome.

2) Since the setting has only 3 possible options, we should use radios instead of a select. There is no need to save space.

3) We have wrong indentation in defaultSettings() and renderOptions() is missing a docblock such as:

  /**
   * Gets the options for the "Render element" setting.
   *
   * @return string[]
   *   The available options.
   */

@bojanz, you are correct in that we usually meet the "Render element" terminology in Drupal documentations or in hook_theme() implementations. Site builders at least, I think, wouldn't expect this term in Drupal's UI - I am not sure if webform uses this anywhere. In any case, the "Wrapper" is more Drupal UI-friendly.

As for the rest, for (3) you are correct. For (2) I don't really have an opinion. Whatever matches the best UX (aka. DX in our case).

Here's my attempt to illustrate #32. Renamed the setting, its label, and its options.

I am still unsure about the "Wrapper type" label.

I did try to rename the options so that both people intimately familiar with Drupal and causal users could figure out what each option does. I haven't used Drupal at my day job for a few years now, and I had to remind myself that "details" is "fieldset, but collapsible", so I was my ideal test user.

Are we supposed to review the patch or the MR now? I like what I see in the screenshot. Is that different then the MR or just a PoC and we still need to roll back into the MR? I'm confused.

My patch is newer than the MR and contains the renames I proposed in #32. i can commit it directly if we agree on the points, we don't need to keep updating the MR.

LGTM

Renamed the setting to be "wrapper_type" so that it matches its label and committed. Right in time for 2.0.0!

Thanks, everyone.

bojanz → closed merge request !34

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