- Issue created by @Liam Morland
- Merge request !7044Issue #3427955: Allow customizing wrapper element in node.html.twig → (Open) created by Liam Morland
- Status changed to Needs review
8 months ago 8:07pm 14 March 2024 - Status changed to Needs work
8 months ago 9:58pm 14 March 2024 - 🇺🇸United States smustgrave
This will need test coverage
Updates to other templates
A change record to announce it.
But also will probably need frontend manager sign off for such a change.
- 🇨🇦Canada Liam Morland Ontario, CA 🇨🇦
Yes, I would like to get approval for this change before going to all the work of implementing it.
- 🇺🇸United States bnjmnm Ann Arbor, MI
FEFM here:
While I'm not against making these kind of optimizations, something like this could result in regressions on existing sites that have styles/tests/etc that expect<article>
. Sites this dependent on specific markup such as this should probably be extending a Stable theme, but I'd still like to be careful that these changes to fundamental templates provide more benefit than risk.Based on the context in MDN page linked in the IS + reviewing some W3 specs, it seems pretty reasonable to interpret the "should" at face value as opposed to it being a "must". I also see that the default scenario is one where a header appears, and that that default scenario is likely to be true by a very high percentage. Based on the MDN recommendation alone, I'm not sure the regression risk is justified. However, I'm also open to changing my mind - my current reluctance is based only on the info currently available to me. If there's further evidence that this is objectively an accessibility bug, or that there are clear benefits that warrant the regression risk, I'm very much open to hearing that.
- 🇨🇦Canada Liam Morland Ontario, CA 🇨🇦
An alternative would be to make the template allow setting the
wrapper_element
in preprocess. If it is not set, usearticle
. This means no change in behaviour, but people can easily make the change if they want without the hassle of overriding the template file. - Status changed to Needs review
2 months ago 2:17pm 17 September 2024 - 🇨🇦Canada Liam Morland Ontario, CA 🇨🇦
I have implemented #7 in the merge request. There is now no change in behaviour except that it it possible to change the wrapper element.
- 🇺🇸United States smustgrave
@bnjmnm should we remove frontend manager tag?
Am moving to NW for test coverage and CR.