klonos
commented
1 year ago @jenlampton this is indeed a huge change to be reviewing. Perhaps if we had a way to compare only commits after the contrib module was added (almost verbatim I'm assuming) as a starting point, then it'd be easier. Anyway, I don't know if there were other commits before the initial one, but I started reviewing one commit at a time as they are listed in https://github.com/backdrop/backdrop/pull/4076/commits
- I liked the removal of
field_group_format_settings_label_validate()and replacing that with making the label field always required 👍🏼 ...it's not 100% match with the previous behavior in the contrib module, but it is much simplified. The only caveat would be when re-saving forms where the label was left blank (because the contrib module allowed that in certain scenarios). Not a biggie though, and perhaps we can get an update hook that is adding some arbitrary labels to field groups with missing labels(?). - Re making the "Separate class names with spaces. ..." description a reusable constant that contrib could reuse, I'd frankly like to get rid of this entire concept. I don't think that it's good UX to be telling people to "leave this field blank to do x" or "do not add a leading forward slash" and if they don't adhere to the help text guidelines, then be throwing errors after form submission 🙄 ...I mean, really? Can't we just remove the leading slash in the form validation? ...so similarly with CSS classes/IDs, I would like us to allow either commas or spaces, and then transparently remove commas in the form/field validation (the same way we
trim()values etc.). That's a more generic follow-up though. -
I like the idea of making the ID field a
'#type' => 'machine_name'...it forbids capital laters though, which although not common, they are still valid CSS. Perhaps add a customreplace_patternof[^a-zA-Z0-9_-]+...in fact. since CSS classes/IDs should not start with a number, the regex should be^[a-zA-Z]{1}[a-zA-Z0-9_-]+$, which ensures that the first character of the class/ID must be a non-numeric character. If we really really want to be accurate, we should tweak that pattern to match the spec in https://www.w3.org/TR/CSS2/syndata.html#characters:- All CSS syntax is case-insensitive within the ASCII range (i.e., [a-z] and [A-Z] are equivalent), except for parts that are not under the control of CSS. For example, the case-sensitivity of values of the HTML attributes "id" and "class", of font names, and of URIs lies outside the scope of this specification. Note in particular that element names are case-insensitive in HTML, but case-sensitive in XML.
- In CSS, identifiers (including element names, classes, and IDs in selectors) can contain only the characters [a-zA-Z0-9] and ISO 10646 characters U+00A0 and higher, plus the hyphen (-) and the underscore (_); they cannot start with a digit, two hyphens, or a hyphen followed by a digit. Identifiers can also contain escaped characters and any ISO 10646 character as a numeric code (see next item). For instance, the identifier "B&W?" may be written as "B\&W\?" or "B\26 W\3F".
...the regex should most likely go into a reusable helper function BTW.
- Speaking of which, I don't think that
#existsis a valid property for'#type' => 'machine_name'- it needs to be anexistskey (note: no#) inside a'#machine_name'property array. If it is valid syntax, then we need to update our FAPI documentation. See: https://docs.backdropcms.org/form_api#machine_name - I would much prefer it if we had separate FAPI elements for CSS classes/IDs, since both core and contrib is using those in settings forms.
- Not sure about the removal of the help text - there are some things that aren't straight-forward, unless you have used field groups before. We could be throwing "dirty form" warnings (for example when accordion items or vertical tabs are "parentless") ...or we could be automagically adding wrapper groups (somehow?). In the meantime, perhaps use the same "collapsed fieldset help" thing that we have also done in the layouts listing page?
That's it for now - I'm sure I must have missed certain things, but once we have the main functionality in, we can iteratively improve/fix things.
jenlampton
bugfolder
quicksketch
olafgrabienski
herbdool
docwilmot
stpaultim
avpaderno
Update 2023:
It has been decided that the field group module provides more options for field groups than we wish to include/support in core, so rather than move the entire module into core, we are planning to revamp the fieldgroup API and move certain types of field groups into core (groupings that already exist in core) and to leave others for the contrib module. The new fieldgroup API will become part of core's field + field_ui modules.
Group types planed for core:
Group types that should remain in contrib:
The Field Group contrib module would simultaneously release a new minor version (that depends on this version of Backdrop core) where those group types are removed from the module, and all remaining types are updated to use the core field group API (which will be similar to what's there already). The current group API will also be removed from the contrib module.
Requirements for getting new fieldgroup contrib module in core:
Original Issue: It would be pretty outstanding if we have field_group in core so that forms could be arranged a lot nicer with things like vertical tabs and such through the UI.
Requirements for getting fieldgroup contrib module in core:
t()function from all testsfield_group.display.testField groupfor the UI and documentation (as opposed toFieldgroupin some places).dsmodule: see https://github.com/backdrop-contrib/ds/issues/11divsettings (what are "effect", and "speed"?)html_elementandhtml5and provide an upgrade pathnew stdClass();in_field_group_get_html_classes().User experience cleanup options:
New features (options):
(is this a good idea?) remove the ability to change the group type from the overview page (why?)
Related issues:
Issue advocate: @jenlampton