Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[4.0] Deprecate unused $iconOver #31211

Merged
merged 11 commits into from
Nov 14, 2020
Merged

[4.0] Deprecate unused $iconOver #31211

merged 11 commits into from
Nov 14, 2020

Conversation

N6REJ
Copy link
Contributor

@N6REJ N6REJ commented Oct 23, 2020

Pull Request for Issue #30211

Summary of Changes

Removes unused $iconOver param.
In 1.5 days this was used as an override ( not hover as stated now ) but has never been implemented in 4.+ so might as well just remove it. If its determined later that its useful we can always create it because as it is now it does NOTHING.

Testing Instructions

notice that toolbar icons display.
apply pr.
make sure toolbar icons still display.
hover over icons notice the icons remain.

Actual result BEFORE applying this Pull Request

nothing noted

Expected result AFTER applying this Pull Request

no visual changes.

Documentation Changes Required

Deprecate $iconOver in 5.x

@N6REJ N6REJ marked this pull request as draft October 23, 2020 04:56
@SharkyKZ
Copy link
Contributor

This can't be removed without rewriting all uses of the method (B/C break) or adding B/C layer based on argument count.

@N6REJ
Copy link
Contributor Author

N6REJ commented Oct 23, 2020

@SharkyKZ well, the thing is its not even used!!!

public static function custom($task = '', $icon = '', $iconOver = '', $alt = '', $listSelect = true, $formId = null)
{
$bar = Toolbar::getInstance('toolbar');
// Strip extension.
$icon = preg_replace('#\.[^.]*$#', '', $icon);
// Add a standard button.
$bar->appendButton('Standard', $icon, $alt, $task, $listSelect, $formId);
}

if you add $iconOver to the appendButton function it becomes a text value in place of the normal text.. i.e. everything is out of sync.
in 3.x it an override NOT hover ( which makes sense since it would be damn hard to create a hover I think )
if you look @ the function its is brought in in the function variables but then does nothing.
image
image
image

I'm open as to what to do because clearly I'd have TONS of icon,icon to change to icon, if I'm to do it 100% correct.
image

As it stands its totally broken.
in 1.5x you'll see its used as an override
#30211 (comment)

In 3.x you'll see its being used as ??
image
image

THAT would be extremely easy to implement though I would still have tons of icon,icon to clean up.

@N6REJ
Copy link
Contributor Author

N6REJ commented Oct 23, 2020

@wilsonge thoughts?

@N6REJ N6REJ marked this pull request as ready for review October 23, 2020 06:56
@N6REJ
Copy link
Contributor Author

N6REJ commented Oct 23, 2020

After discussing @ length with @SharkyKZ we decided to leave the variable but deprecate its behavior.

@hans2103
Copy link
Contributor

People might call for JToolbarHelper::custom('something', 'something', '', 'alt-text');. Removing it now will break b/c.
To make it deprecate is a good point. You can safely remove it in the next J version.

@brianteeman
Copy link
Contributor

This should be closed #31211 (comment) and a new pr opened for the deprecation

@N6REJ
Copy link
Contributor Author

N6REJ commented Oct 23, 2020

This should be closed #31211 (comment) and a new pr opened for the deprecation

Thats what this is!

@N6REJ N6REJ changed the title remove unused $iconOver [4.0] Deprecate unused $iconOver Oct 23, 2020
@N6REJ N6REJ requested a review from wilsonge October 23, 2020 19:17
@brianteeman
Copy link
Contributor

It is now you changed the title :)

…conover-4

� Conflicts:
�	administrator/components/com_templates/src/View/Template/HtmlView.php
@Quy
Copy link
Contributor

Quy commented Oct 31, 2020

I have tested this item ✅ successfully on 29a54db


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/31211.

@jwaisner
Copy link
Member

jwaisner commented Nov 2, 2020

I have tested this item ✅ successfully on 70cbc49


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/31211.

@Quy
Copy link
Contributor

Quy commented Nov 2, 2020

RTC


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/31211.

@joomla-cms-bot joomla-cms-bot added the RTC This Pull Request is Ready To Commit label Nov 2, 2020
@richard67 richard67 merged commit 5a6616d into joomla:4.0-dev Nov 14, 2020
@joomla-cms-bot joomla-cms-bot removed the RTC This Pull Request is Ready To Commit label Nov 14, 2020
@richard67
Copy link
Member

Thanks!

@richard67 richard67 added this to the Joomla 4.0 milestone Nov 14, 2020
@N6REJ N6REJ deleted the iconover-4 branch November 14, 2020 14:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants