[#17646] - [com_fields] Unify rendering
- Closed
- 18 Aug 2018
- Medium
- Build: staging
- # 17646
- Diff
- matrikular:contactx
User tests: Successful: Unsuccessful:
Pull Request for Issue #16594. Further references: PR #17636, PR #15968
Summary of Changes
- Unify custom fields rendering for com_contact and com_fields (keeping it as close to what it has been as possible).
- Fix obsolete whitespaces in field class attribute
- Refactoring / Cleanup variables, conditions and remove unused code blocks.
- Add and unify escaping to all output (labels / values).
Testing Instructions
- Create one or more custom fields in the context of both com_contact (single contact) and com_content (single article).
- Create a menu item to each of those entities.
- Inspect the rendered HTML markup.
Expected result
The HTML markup for custom fields in both views (contact / article) should look something like this:
Articles after patch:
<dl class="fields-container">
<dt class="field-entry">
<span class="field-label">Custom Article Field:</span>
</dt>
<dd class="field-entry">
<span class="field-value">Custom Article Field</span>
</dd>
</dl>
Contacts after patch:
<dl class="fields-container contact-fields dl-horizontal">
<dt class="contact-field-entry">
<span class="field-label">Custom Contact Field:</span>
</dt>
<dd class="contact-field-entry">
<span class="field-value">Custom Contact Field</span>
</dd>
</dl>
Actual result
- The rendered HTML markup shows obsolete whitespace in a fields class attributes when no render class was entered.
- The markup for custom fields in the context of articles is missing* the dt tag and instead renders both "label" and "value" inside a dd tag.
Articles before patch:
<dl class="fields-container">
<dd class="field-entry ">
<span class="field-label">Custom Article Field: </span>
<span class="field-value">Custom Article Field</span>
</dd>
</dl>
Contacts before patch:
<dl class="fields-container contact-fields dl-horizontal">
<dt class="contact-field-entry ">
<span class="field-label">Custom Contact Field: </span>
</dt>
<dd class="contact-field-entry ">
<span class="field-value">Custom Contact Field</span>
</dd>
</dl>
Notes
It seems to be a good choice to make use of a definition list instead of an unordered list here, but we're runing into a dilemma when a fields label was set to "hide". Before the patch, com_content wouldn't have rendered a dt tag at all, though it is required, while com_contact on the other hand, just renders an empty dt tag.
Edit: The validator seems to be fine with an empty dt tag, so I guess we go with that for now.
joomla-cms-bot
- | Category | ⇒ | Front End com_contact com_fields |
| Status | New | ⇒ | Pending |
| Labels |
Added:
?
|
||
@franz-wohlkoenig thank you for testing. This is expected behavior as the mandatory dt tag was missing from the markup before the patch.
The dl element represents an association list consisting of zero or more name-value groups (a description list). A name-value group consists of one or more names (dt elements) followed by one or more values (dd elements), ignoring any nodes other than dt and dd elements. Within a single dl element, there should not be more than one dt element for each name.
I have tested this item
In com_contact display looks with PR like:
So this should work also in com_content.
This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/17646.
Why all the stdClass checks? Did you ever had the case where we didn't get a stdclass?
I was about to open a RFC about the current implementation in general (fields, layouts, $displayData, and so on) but thought it would go too far in the context of this PR.
As long as there is no "contract" in place that makes sure the passed field has all the properties that are accessed in the respective field/s render layout, there will be a (small) chance for a notice / warning to be generated and / or the layout not behaving as intended.
It just does not feel very comforting that we rely on an object that comes either from $displayData['fields'], $item->jcfields, FieldsHelper::getFields(), was passed to and maybe modified by plugins and / or overrides.
Since the key_exists function would not check if an item was populated, is of a certain type and / or an object for that matter, the check for stdClass is meant to prevent at least an array to be passed and could "later" maybe exchanged for something even more strict (Instance of FormField?). That said, I don't know the master plan for all of this, which is why I reached out to you in Glip.
If you "promise" ;-) that there are and always will be (populated) objects of type X (items, fields), then we can get rid of the check, of course.
I have tested this item
The PR generates correct html strcture for the Definition List. Tested on Article only, as @franz-wohlkoenig has tested in com_contact.
This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/17646.
I tested the correctness of the generated html. When I use Definition Lists in an older Template with overrides I use css for styling. We also do not know which bootstrap version is in use. But I see the issue.
@matrikular I'm asking this because the JLayouts should be simple, in best case only HTML output and no PHP code. If the data is not passed correctly to the layout then it is the job of the extension to make sure the data is in the format the layout needs it. That's why I'm asking and I want to prevent that the layouts become more complex than really needed.
@laoneo I know that. Refactoring PHP code / logic, getting rid of unused code blocks and unnecessary variable initialisation + unifying markup is exactly what I did to get there. Who in his / her right mind would change a file for any other reason? As I've said, with no "contract" in place and dealing with plain objects, this seems to be what we have to work with in the current layout system. I just exchanged one condition in that place with a more strict one.
On a (rhetorical) side note: while keeping logic and output apart (for obvious reasons), we should stop implying that the "frontend guy" can not handle some lines of PHP code. Todays frontend tool stack with its build tools, pre-compilers and the like are equaly, if not more complex than some simple if constructs or variable concatenations.
Again, if there is any plan in "your" drawer to optimise the system even further, please let me hear about it.
P.S. Can someone please restart drone?
No there is no master plan.
that the "frontend guy" can not handle some lines of PHP code
All what I want to say is that need to keep the layouts as less complex as possible.
| Status | Pending | ⇒ | Closed |
| Closed_Date | 0000-00-00 00:00:00 | ⇒ | 2017-09-01 19:43:17 |
| Closed_By | ⇒ | matrikular |
| Status | Closed | ⇒ | New |
| Closed_Date | 2017-09-01 19:43:17 | ⇒ | |
| Closed_By | matrikular | ⇒ |
| Status | New | ⇒ | Pending |
@matrikular can you please solve Conflicts so People can test?
| Title |
|
||||||
As it has been over nine months since the request for updates and conflict resolution without any update I am closing this.. It can always be re-opened if updated.
| Status | Pending | ⇒ | Closed |
| Closed_Date | 0000-00-00 00:00:00 | ⇒ | 2018-08-18 12:56:26 |
| Closed_By | ⇒ | brianteeman |
I have tested this item? unsuccessfully on 164565a
com_content before Patch:
com_content after Patch:
Issue: Label and Value are not in same Line.
This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/17646.