? Pending

User tests: Successful: Unsuccessful:

avatar mattiaverga
mattiaverga
15 Oct 2017

Pull Request for Issue #18342 .

Summary of Changes

This modify the default fieldgroup layout to use a new field layout with two different <dt> and <dd> elements for label and value.
Since we don't have a master <dd> element container, the custom class set in backend is applied to both the elements.

Testing Instructions

Create a fieldgroup, publish it inside an article and look at the output.

Expected result

<dl>
<dt>label<\dt>
<dd>value<\dd>
<\dl>

Actual result

<dl>
<dd><span>label<\span>: <span>value<\span><\dd>
<\dl>
avatar joomla-cms-bot joomla-cms-bot - change - 15 Oct 2017
Category Front End com_fields
avatar mattiaverga mattiaverga - open - 15 Oct 2017
avatar mattiaverga mattiaverga - change - 15 Oct 2017
Status New Pending
avatar laoneo
laoneo - comment - 16 Oct 2017

If you want to change the markup then I would target this pr against 4.0 as for me it is a kind of BC break (I know, not covered by the terms), but it would break all sites which are adapted to the old markup.

avatar mattiaverga
mattiaverga - comment - 16 Oct 2017

Well, yes this PR changes the default fieldgroup markup (but not the default single field rendering). Anyway, the current way to render the fieldgroup is wrong and doesn't validate standards.
Is targeting the PR against 3.9-dev acceptable?

avatar mbabker
mbabker - comment - 16 Oct 2017

Per the B/C strategy:

We will try not to change markup in such a way that a site might render differently, but we can't promise not to break anything at the present time.

In this case it looks like we're dealing with moving an invalid markup structure to a valid one. So I think it'd be acceptable to make that change without waiting for 4.0. But, as it does have potential concerns with page structures, I would suggest it go into 3.9 with a note about the structure change (even if it is relatively minor) versus going into a 3.8 patch release.

avatar laoneo
laoneo - comment - 16 Oct 2017

Like it is now is to be inline with com_content. For example in com_contact we made a layout override for it to have a similar structure as you propose.
So if this change is going to be accepted a layout override for com_content needs to be done and the one of com_contact adapted.

avatar mattiaverga mattiaverga - change - 16 Oct 2017
Title
Render fieldgroups as a pair of <dt> and <dd>
Render fieldgroups as a pair of and
Labels Added: ?
avatar mattiaverga
mattiaverga - comment - 16 Oct 2017

@laoneo I don't understand why com_content should need a layout override. It just uses the default layout provided by com_fields.

Anyway, I made two mistakes: my previous PR wasn't right and I reworked it. The list.php layout for single field didn't had sense (it was not standard compliant to have a <dt><dd> pair without a <dl> container).
The second mistake is that a <dl> with a single <dd> is not violating standards, so the actual way to render fieldgroups is ok (but I think my PR would make things better).

avatar laoneo
laoneo - comment - 17 Oct 2017

Because com_content displays the info block with the pattern com_fields has now. If we are going to change that, then it doesn't look like it would nicely integrated.

avatar mattiaverga
mattiaverga - comment - 17 Oct 2017

To make com_content display fieldgroups better, I was planning to add two new layout overrides, unordered list and table. I'm unsure if I should push them to a new PR or if I can add them here.

If changing the default com_fields rendering can be problematic, I can cancel this PR and just add the new overrides to com_content (without changing the default). Maybe this is the best option.

avatar mattiaverga
mattiaverga - comment - 20 Oct 2017

I've just found that my PR it's a duplicate of #17646 , so I'm closing it.

avatar mattiaverga mattiaverga - change - 20 Oct 2017
Status Pending Closed
Closed_Date 0000-00-00 00:00:00 2017-10-20 16:35:32
Closed_By mattiaverga
avatar mattiaverga mattiaverga - close - 20 Oct 2017
avatar franz-wohlkoenig
franz-wohlkoenig - comment - 20 Oct 2017

@mattiaverga can you please test #17646?

avatar mattiaverga
mattiaverga - comment - 21 Oct 2017

@franz-wohlkoenig should I test it even if it actually has conflicts?

avatar franz-wohlkoenig
franz-wohlkoenig - comment - 21 Oct 2017

i wrote now at #17646 to solve Conflicts.

Add a Comment

Login with GitHub to post a comment