[4.0] [JLayout] merge two equal JLayouts for iconclass into one new JLayout for iconclass #30470

hans2103 · 2020-08-24T21:49:51Z

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

brianteeman · 2020-08-25T00:01:48Z

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?

hans2103 · 2020-08-25T06:28:00Z

@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.

brianteeman · 2020-08-25T06:43:12Z

Files being identical is a consequence of the layouts concept.

Can the button still be overridden separately to the toolbar?

hans2103 · 2020-08-25T07:26:31Z

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

layouts/joomla/icon/iconclass.php

# Conflicts: # libraries/src/HTML/Helpers/JGrid.php

layouts/joomla/icon/iconclass.php

brianteeman · 2020-08-25T21:24:11Z

@hans2103 much better now - thanks

N6REJ · 2020-08-26T00:45:22Z

nice to have @hans2103 working with me to take this project to the next step.

Quy · 2020-08-26T17:33:13Z

@N6REJ Close PR #30482?

N6REJ · 2020-08-27T03:56:08Z

@N6REJ Close PR #30482?
as I said in that pr, no, because they don't exactly replace each other. @hans2103 and I are working together and they will be merged appropiately

Quy · 2020-08-28T15:40:49Z

I have tested this item ✅ successfully on a62d5b5

_{This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/30470.}

jwaisner · 2020-08-28T21:02:46Z

I have tested this item ✅ successfully on a62d5b5

_{This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/30470.}

jwaisner · 2020-08-28T21:03:01Z

RTC

_{This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/30470.}

N6REJ · 2020-08-31T14:53:14Z

I have tested this item ✅ successfully on 1b4e36b

#30468

and

#30482

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.}

libraries/src/HTML/Helpers/ActionsDropdown.php

@N6REJ

@N6REJ thank you

libraries/src/HTML/Helpers/ActionsDropdown.php

Co-authored-by: Richard Fath <richard67@users.noreply.github.com>

libraries/src/HTML/Helpers/ActionsDropdown.php

N6REJ · 2020-09-04T03:41:15Z

I have tested this item ✅ successfully on 0d1156a

works as expected

_{This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/30470.}

layouts/joomla/icon/iconclass.php

I like this PHP7 operator Co-authored-by: Quy <quy@fluxbb.org>

# Conflicts: # layouts/joomla/content/info_block/category.php

…103/joomla-cms into feature/layouthelper-for-dry-icons

hans2103 · 2020-09-06T14:23:44Z

merged recent 4.0-dev updates and solved merge conflict

ghost · 2020-09-07T06:21:58Z

I have tested this item ✅ successfully on b00f232

_{This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/30470.}

paternax · 2020-09-07T07:26:33Z

I have tested this item ✅ successfully on b00f232

_{This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/30470.}

paternax · 2020-09-07T07:26:36Z

I have tested this item ✅ successfully on b00f232

_{This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/30470.}

richard67 · 2020-09-07T09:50:15Z

RTC

_{This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/30470.}

infograf768 · 2020-09-11T08:42:06Z

Thanks

@Quy

…Layout for iconclass (joomla#30470) * 🎨 merge two equal JLayouts into one new JLayout * replace hard coded icon to call for JLayout * replace icon mapping to call for JLayout * add icon protected to layout * icon protected moved to jlayout iconclass * remove fas * add icon trash to mapping * add parameter to check if html should be rendered or icon class only * simplify php * replace massive if-then-elseif-then by switch * merge duplicate outcome fa-times * be consistent * simplify switch thx @Quy * copy changes from https://github.com/joomla/joomla-cms/pull/30468/files * copy changes from https://github.com/joomla/joomla-cms/pull/30468/files * remove setting icon to itself * copy changes from joomla#30468 * change to use layout * Update libraries/src/HTML/Helpers/ActionsDropdown.php Co-authored-by: Richard Fath <richard67@users.noreply.github.com> * use LayoutHelper in function * let default html output be true, instead of false * Update layouts/joomla/icon/iconclass.php I like this PHP7 operator Co-authored-by: Quy <quy@fluxbb.org> Co-authored-by: Troy T. Hall <programming@hallhome.us> Co-authored-by: Richard Fath <richard67@users.noreply.github.com> Co-authored-by: Quy <quy@fluxbb.org>

hans2103 added 6 commits August 24, 2020 23:11

🎨 merge two equal JLayouts into one new JLayout

c3c767e

replace hard coded icon to call for JLayout

190f531

replace icon mapping to call for JLayout

9e501b0

add icon protected to layout

3e4a662

icon protected moved to jlayout iconclass

afa0337

remove fas

34d0100

joomla-cms-bot added the PR-4.0-dev label Aug 24, 2020

hans2103 changed the title ~~Feature/layouthelper for dry icons~~ [4.0] [JLayout] merge two equal JLayouts for iconclass into one new JLayout for iconclass Aug 24, 2020

hans2103 added 3 commits August 24, 2020 23:59

add icon trash to mapping

62a5d2e

add parameter to check if html should be rendered or icon class only

df94753

simplify php

fe8fb76

hans2103 mentioned this pull request Aug 25, 2020

convert icon-trash to fas fa-trash #30468

Closed

Quy reviewed Aug 25, 2020

View reviewed changes

layouts/joomla/icon/iconclass.php Outdated Show resolved Hide resolved

hans2103 added 2 commits August 25, 2020 17:53

replace massive if-then-elseif-then by switch

0d179b0

Merge branch '4.0-dev' into feature/layouthelper-for-dry-icons

a790cf7

# Conflicts: # libraries/src/HTML/Helpers/JGrid.php

Quy reviewed Aug 25, 2020

View reviewed changes

layouts/joomla/icon/iconclass.php Outdated Show resolved Hide resolved

hans2103 added 3 commits August 25, 2020 18:49

merge duplicate outcome fa-times

50db3a5

be consistent

adcbee7

simplify switch thx @Quy

a62d5b5

N6REJ mentioned this pull request Aug 31, 2020

[4.0] Convert jgrid if (icon) to switch (icon) #30482

Closed

Quy reviewed Sep 1, 2020

View reviewed changes

libraries/src/HTML/Helpers/ActionsDropdown.php Outdated Show resolved Hide resolved

N6REJ and others added 3 commits September 2, 2020 17:41

change to use layout

9fbb5c5

Merge branch 'feature/layouthelper-for-dry-icons' into hans

0bcd5cd

Merge pull request #1 from N6REJ/hans

9af7b36

@N6REJ thank you

richard67 reviewed Sep 3, 2020

View reviewed changes

libraries/src/HTML/Helpers/ActionsDropdown.php Outdated Show resolved Hide resolved

Update libraries/src/HTML/Helpers/ActionsDropdown.php

e1e81ff

Co-authored-by: Richard Fath <richard67@users.noreply.github.com>

Quy reviewed Sep 3, 2020

View reviewed changes

libraries/src/HTML/Helpers/ActionsDropdown.php Outdated Show resolved Hide resolved

use LayoutHelper in function

0d1156a

hans2103 mentioned this pull request Sep 5, 2020

Feature/4.0 dev a11y com finder refactor search result #30550

Merged

let default html output be true, instead of false

b6eb15f

Quy reviewed Sep 6, 2020

View reviewed changes

layouts/joomla/icon/iconclass.php Outdated Show resolved Hide resolved

hans2103 and others added 3 commits September 6, 2020 16:18

Update layouts/joomla/icon/iconclass.php

f0f2b20

I like this PHP7 operator Co-authored-by: Quy <quy@fluxbb.org>

Merge branch '4.0-dev' into feature/layouthelper-for-dry-icons

c7c05c3

# Conflicts: # layouts/joomla/content/info_block/category.php

Merge branch 'feature/layouthelper-for-dry-icons' of github.com:hans2…

b00f232

…103/joomla-cms into feature/layouthelper-for-dry-icons

joomla-cms-bot added the RTC This Pull Request is Ready To Commit label Sep 7, 2020

infograf768 merged commit 0d6db34 into joomla:4.0-dev Sep 11, 2020

joomla-cms-bot removed the RTC This Pull Request is Ready To Commit label Sep 11, 2020

infograf768 added this to the Joomla 4.0 milestone Sep 11, 2020

hans2103 deleted the feature/layouthelper-for-dry-icons branch September 11, 2020 09:43

hans2103 mentioned this pull request Sep 14, 2020

[4.0][DRAFT] replace hard coded icon by jlayout #30634

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[4.0] [JLayout] merge two equal JLayouts for iconclass into one new JLayout for iconclass #30470

[4.0] [JLayout] merge two equal JLayouts for iconclass into one new JLayout for iconclass #30470

hans2103 commented Aug 24, 2020 •

edited

brianteeman commented Aug 25, 2020

hans2103 commented Aug 25, 2020

brianteeman commented Aug 25, 2020

hans2103 commented Aug 25, 2020

brianteeman commented Aug 25, 2020

N6REJ commented Aug 26, 2020

Quy commented Aug 26, 2020

N6REJ commented Aug 27, 2020

Quy commented Aug 28, 2020

jwaisner commented Aug 28, 2020

jwaisner commented Aug 28, 2020

N6REJ commented Aug 31, 2020

N6REJ commented Sep 4, 2020

hans2103 commented Sep 6, 2020

ghost commented Sep 7, 2020

paternax commented Sep 7, 2020

paternax commented Sep 7, 2020

richard67 commented Sep 7, 2020

infograf768 commented Sep 11, 2020

[4.0] [JLayout] merge two equal JLayouts for iconclass into one new JLayout for iconclass #30470

[4.0] [JLayout] merge two equal JLayouts for iconclass into one new JLayout for iconclass #30470

Conversation

hans2103 commented Aug 24, 2020 • edited

Summary of Changes

Testing Instructions

Actual result BEFORE applying this Pull Request

Expected result AFTER applying this Pull Request

Documentation Changes Required

Examples

call for className and as HTML output

call for className and className output

call for stateName and as HTML output

call for stateName and as className output

brianteeman commented Aug 25, 2020

hans2103 commented Aug 25, 2020

brianteeman commented Aug 25, 2020

hans2103 commented Aug 25, 2020

brianteeman commented Aug 25, 2020

N6REJ commented Aug 26, 2020

Quy commented Aug 26, 2020

N6REJ commented Aug 27, 2020

Quy commented Aug 28, 2020

jwaisner commented Aug 28, 2020

jwaisner commented Aug 28, 2020

N6REJ commented Aug 31, 2020

N6REJ commented Sep 4, 2020

hans2103 commented Sep 6, 2020

ghost commented Sep 7, 2020

paternax commented Sep 7, 2020

paternax commented Sep 7, 2020

richard67 commented Sep 7, 2020

infograf768 commented Sep 11, 2020

hans2103 commented Aug 24, 2020 •

edited