Add method to set document head (meta) data #19234
Conversation
| } | ||
|
|
||
| if ($parent == false) | ||
| if ($category == false || $parent == false) |
There was a problem hiding this comment.
is it correct to be == here but === elsewhere
There was a problem hiding this comment.
It is from original code, I just merged two commands into one. If it is needed, I can change it later. The main thing I want to get feedback is whether this concept is accepted (and useful) or not before working to use it for our view classes (to reduce repeating code).
libraries/src/MVC/View/HtmlView.php
Outdated
| * | ||
| * @return void | ||
| * | ||
| * @since 3.9.0 |
libraries/src/MVC/View/HtmlView.php
Outdated
|
|
||
| $title = $params->get('page_title'); | ||
|
|
||
| // Check for empty title and add site name if param is set |
There was a problem hiding this comment.
Replace lines 897-911 with setDocumentTitle method.
There was a problem hiding this comment.
It is my intention to have these lines of code in the method. If this is accepted, we can set all these head data within single method, no need for using setDocumentTitle method anymore
libraries/src/MVC/View/HtmlView.php
Outdated
| * | ||
| * @since __DEPLOY_VERSION__ | ||
| */ | ||
| protected function setDocumentHeadDatas() |
There was a problem hiding this comment.
Data is considered singular/plural. Perhaps change it to setDocumentHeadData.
|
You can use method exists inside same class, so your setDocumentHeadData() could make use of it |
|
Yes, I am aware of that method. I just think maybe we don't need that method in the future, so sometime it can be deprecated and can be removed from the class in the future |
|
Reasoning for adding setDocumentTitle($title) is same as adding setDocumentHeadData() |
|
About removing it
Also It is probably best to completly remove the "title" code from setDocumentHeadData() $this->setDocumentTitle();
$this->setDocumentHeadData(); // or maybe "setDocumentMeta()" or somethingThus you achieve both replacing the repeatable code, |
|
I don't think it is good to call setDocumentHeadData and then call setDocumentTitle($title) to override only the title. If we do that, we would have the same code run two times, it is a waste of resource. It is better to call $this->params->set('page_title', $title) before calling setDocumentHeadData(); If we have a use case which we only need to set page title, not other meta data like description, robots..., on a View class, then Yes, we should keep setDocumentTitle and use it on setDocumentHeadData. Otherwise, I would like to keep the code as how it is at the moment. |
|
yes, that s why i said that my 2nd suggestion is better, |
|
I still prefer one method call per view only. I don't like having to call two methods: $this->setDocumentTitle();
$this->setDocumentHeadData(); Can we get feedback from PLT about this? |
libraries/src/MVC/View/HtmlView.php
Outdated
| $this->document->setMetadata('robots', $params->get('robots')); | ||
| } | ||
|
|
||
| $metaData = $this->params->get('metadata'); |
There was a problem hiding this comment.
You can pass it before calling this method. For your information, we are using it here https://github.com/joomla/joomla-cms/blob/staging/components/com_content/views/category/view.html.php#L250-L258
|
Can't say I'm a fan of this. It's introducing more undocumented class member variables into a base class we already have problems with having undocumented variables on. And it's making a hard reservation on the |
|
@mbabker I change it to pass $params as method parameter. Hope it is OK now. |
|
@mbabker are the changes better now or are your comments still valid in which case this should be closed |
|
Seems fine as is |
Pull Request for Issue # .
Summary of Changes
In view classes of a component, we usually have to set document head data like page title, description, meta keywords, meta description, robots...
Instead of writing that repeating code again and again in every view, this PR introduces a new method called setDocumentHeadDatas to do that work. Component view classes now only need to set necessary data to $params property of the view if needed and call setDocumentHeadDatas method to set these data for document
Testing Instructions
Code review for now. If the concept is accepted, I will convert actual view classes (like category view) to use this new method and human testing will be needed at that time.