[#30470] - [4.0] [JLayout] merge two equal JLayouts for iconclass into one new JLayout for iconclass
- Fixed in Code Base
- 11 Sep 2020
- Medium
- Build: 4.0-dev
- # 30470
- Diff
- hans2103:feature/layouthelper-for-dry-icons
User tests: Successful: Unsuccessful:
This PR has influence on #30468 and #30464
Summary of Changes
This PR will prevent duplicate code.
Both layouts/joomla/button/iconclass.php and layouts/joomla/toolbar/iconclass.php are the same.
Merging both will make our code a bit DRY
Testing Instructions
Apply the PR and go to Joomla administrator or Joomla frontend and look for icons on buttons or in a toolbar.
There should be no change visible before or after applying the PR.
Actual result BEFORE applying this Pull Request
Buttons in the toolbar have icons
Expected result AFTER applying this Pull Request
Buttons in the toolbar have icons
Documentation Changes Required
The newly created Layout joomla.icon.iconclass has two parameters
- icon => name or className => the icon that should be displayed
- html => true/false to display html output or className only
Examples
call for className and as HTML output
use Joomla\CMS\Layout\LayoutHelper;
echo LayoutHelper::render('joomla.icon.iconclass', ['icon' => 'fa-folder-open', 'html' => true]);
this will render
<span class="fas fa-folder-open" aria-hidden="true"></span>
call for className and className output
use Joomla\CMS\Layout\LayoutHelper;
echo LayoutHelper::render('joomla.icon.iconclass', ['icon' => 'fa-folder-open']);
this will render
fas fa-folder-open
call for stateName and as HTML output
use Joomla\CMS\Layout\LayoutHelper;
echo LayoutHelper::render('joomla.icon.iconclass', ['icon' => 'published', 'html' => true ]);
this will render
<span class="fas fa-check" aria-hidden="true"></span>
call for stateName and as className output
use Joomla\CMS\Layout\LayoutHelper;
echo LayoutHelper::render('joomla.icon.iconclass', ['icon' => 'published' ]);
this will render
fas fa-check
| Status | New | ⇒ | Pending |
joomla-cms-bot
- | Category | ⇒ | Layout Libraries |
| Title |
|
||||||
| Labels |
Added:
?
|
||
@brianteeman This PR reduces duplicate code but keeps the possibility to override the origin.
Developers still can both toolbar/iconclass.php and button/iconclass.php the way they did before.
Both files echo the icon/iconclass.php
Both files where exactly the same and there is no use of maintaining two equal files.
We should try not to repeat ourself while coding.
content of layouts/joomla/toolbar/iconclass.php === content of layouts/joomla/button/iconclass.php
Only that should be reason enough to merge it into one file.
Files being identical is a consequence of the layouts concept.
Can the button still be overridden separately to the toolbar?
Can the button still be overridden separately to the toolbar?
yes.. I haven't deleted both files. The only echo the new layout. You can override button/iconclass to your own template and go wild in the code. Same goed for toolbar/iconclass
I have tested this item
This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/30470.
I have tested this item
This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/30470.
| Status | Pending | ⇒ | Ready to Commit |
RTC
This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/30470.
| Labels |
Added:
?
|
||
| Status | Ready to Commit | ⇒ | Pending |
Back to pending status as requested by the author.
This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/30470.
| Labels |
Removed:
?
|
||
I have tested this item
#30468
and
now merged with this pr so will close them.
This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/30470.
I have tested this item
works as expected
This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/30470.
merged recent 4.0-dev updates and solved merge conflict
I have tested this item
This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/30470.
I have tested this item
This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/30470.
I have tested this item
This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/30470.
| Status | Pending | ⇒ | Ready to Commit |
RTC
This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/30470.
| Status | Ready to Commit | ⇒ | Fixed in Code Base |
| Closed_Date | 0000-00-00 00:00:00 | ⇒ | 2020-09-11 08:41:59 |
| Closed_By | ⇒ | infograf768 | |
| Labels |
Added:
?
|
||
Thanks
Clearly the current code is not optimal but is merging the layouts the solution? I thought the whole point of layouts was that they were single purpose and could be overridden. Doesn't this change prevent that?