? ? Pending

User tests: Successful: Unsuccessful:

avatar N6REJ
N6REJ
8 Sep 2020

Pull Request for Issue # .

Summary of Changes

Icon array is titled "image". This changes it to icon to be more consistent with its function.

Testing Instructions

inspect 4.0-dev dashboard. Verify quickicons are showing.
image
apply pr.
verify icons still display properly.

Actual result BEFORE applying this Pull Request

image

Expected result AFTER applying this Pull Request

image

Documentation Changes Required

b/c as it may require third party devs to change the $displayData['image'] to $displayData['icon']

avatar N6REJ N6REJ - open - 8 Sep 2020
avatar N6REJ N6REJ - change - 8 Sep 2020
Status New Pending
avatar joomla-cms-bot joomla-cms-bot - change - 8 Sep 2020
Category Modules Administration Layout
avatar brianteeman
brianteeman - comment - 8 Sep 2020

After applying the patch the Update checks block is missing any icon

image

image

avatar brianteeman brianteeman - test_item - 8 Sep 2020 - Tested unsuccessfully
avatar brianteeman
brianteeman - comment - 8 Sep 2020

I have tested this item ? unsuccessfully on c4fdf39


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

avatar N6REJ
N6REJ - comment - 8 Sep 2020

@brianteeman i'll check on that after the doctors in about 12hrs. Sorry about that.

avatar chmst
chmst - comment - 8 Sep 2020

The name is inherited from J3. 3rd party developers can add fotos or vectors to their quickicons.


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

avatar brianteeman
brianteeman - comment - 8 Sep 2020

@chmst that was what I was worried about which is why I had akeeba installed to check

avatar N6REJ
N6REJ - comment - 8 Sep 2020

@brianteeman @chmst I see what you guys mean.


		$ret = [
			'link'  => 'index.php?option=com_akeeba&view=Backup&autostart=1&returnurl=' . base64_encode($url) . '&profileid=' . $profileId . "&$token=1",
			'image' => 'akeeba-black',
			'text'  => Text::_('PLG_QUICKICON_AKEEBABACKUP_OK'),
			'id'    => 'plg_quickicon_akeebabackup',
			'group' => 'MOD_QUICKICON_MAINTENANCE',
		];

		if (version_compare(JVERSION, '3.0', 'lt'))
		{
			$ret['image'] = $url . '/../media/com_akeeba/icons/akeeba-48.png';
		}

HOWEVER,
in the core quickicon plugins we have both icon & image and Icon is never used in the layout.

SO, we have a b/c either way ?
@nikosdion as a major developer, is it reasonable to replace the array item name? Or am I barking up the wrong tree?

avatar N6REJ N6REJ - change - 8 Sep 2020
Labels Added: ?
avatar joomla-cms-bot joomla-cms-bot - change - 8 Sep 2020
Category Modules Administration Layout Modules Administration Language & Strings Layout Front End Plugins
avatar nikosdion
nikosdion - comment - 8 Sep 2020

Can you please stop changing random things just for change's sake? Every time something like that is changing with no rhyme or reason us 3PDs have to waste our time writing workarounds.

And please stop at-mentioning me. It's annoying and a waste of my time. As long as certain people are in the project who consider my every opinion wrong by default -- even when they end up doing what I suggested, promoting it as their own superbly novel and fantastic idea -- there's no point in me participating. I have better things to do.

avatar N6REJ N6REJ - change - 8 Sep 2020
The description was changed
avatar N6REJ N6REJ - edited - 8 Sep 2020
avatar N6REJ
N6REJ - comment - 8 Sep 2020

@nikosdion don't think the sarcasm was warranted. I asked out of respect for your efforts. You shouldn't imply mine have less value or intent.

avatar nikosdion
nikosdion - comment - 8 Sep 2020

@N6REJ Thank you for proving my point. It doesn't matter what I say, you will all find a way to be offended by something I DID NOT EVEN SAY. So please stop wasting my time!

avatar brianteeman
brianteeman - comment - 8 Sep 2020

@N6REJ There are many extensions that provide quickicons. I just tested with the one I use all the time. This proposed change is just cosmetic, there is no benefit to the change. So as it breaks b/c for no benefit I suggest that it is closed.

avatar N6REJ
N6REJ - comment - 10 Sep 2020

@brianteeman I agree with you that its trivial. We're discussing this in the team and have a solution that will work. We're working on the entire icon system that is a huge mess right now, so there are going to be changes that might seem trivial but their goal is to make the entire system make sense.

avatar Bakual
Bakual - comment - 10 Sep 2020

Leave it as image please.
An icon is an image as well - so it isn't wrong. And it was named like this for a long time and nobody had problems understanding what it is used for.

avatar brianteeman
brianteeman - comment - 10 Sep 2020

If its not broken dont improve it so that you can break it

avatar N6REJ
N6REJ - comment - 10 Sep 2020

Leave it as image please.
An icon is an image as well - so it isn't wrong. And it was named like this for a long time and nobody had problems understanding what it is used for.

The main issue is, there is both image and icon and yet icon is never used. Makes no sense to have both.

avatar Bakual
Bakual - comment - 10 Sep 2020

The main issue is, there is both image and icon and yet icon is never used. Makes no sense to have both.

Just delete the never used icon one.

avatar N6REJ
N6REJ - comment - 12 Sep 2020

everywhere else we use $displayData['icon'], its very inconsistent to not do so here.
But we'll address this another way.

avatar N6REJ N6REJ - change - 12 Sep 2020
Status Pending Closed
Closed_Date 0000-00-00 00:00:00 2020-09-12 01:11:01
Closed_By N6REJ
Labels Added: ?
avatar N6REJ N6REJ - close - 12 Sep 2020
avatar N6REJ N6REJ - change - 14 Sep 2020
Status Closed New
Closed_Date 2020-09-12 01:11:01
Closed_By N6REJ
avatar N6REJ N6REJ - change - 14 Sep 2020
Status New Pending
avatar N6REJ N6REJ - reopen - 14 Sep 2020
avatar N6REJ N6REJ - change - 14 Sep 2020
Title
[4.0] Convert array name from 'image' to 'icon'
[4.0] [DRAFT] Mod_quickicon normalization
avatar N6REJ N6REJ - edited - 14 Sep 2020
avatar HLeithner
HLeithner - comment - 14 Sep 2020

As nicholas already said, don't change 'names' only because they maybe look wrong. We would need to deprecated the old one and force 1000 of developers to change there components without any gain.

PS: this switch statement looks really weird

avatar brianteeman
brianteeman - comment - 15 Sep 2020

This really should be closed.

avatar N6REJ
N6REJ - comment - 15 Sep 2020

@brianteeman This is going to be rewritten in a way that fully supports both methods. Which is why its in draft mode.
The fact that we are too lazy to fully implement a method that was created 8yrs ago makes us look horrible.
With any luck the old method will be deprecated for J5.

avatar brianteeman
brianteeman - comment - 15 Sep 2020

So close this and create a new one or reopen this when you have another attempt.

avatar Bakual
Bakual - comment - 16 Sep 2020

Just delete the icon and use the image. There is nothing wrong with that, it's clear what it does and nobody had an issue with it for years. The code is not a novel where the choice of words matter. This is code - it's far more important to not touch it when it's not broken,

avatar richard67
richard67 - comment - 17 Sep 2020

@N6REJ Please don't get me wrong, but in this case, for this PR here, not for the other icon stuff, I agree with what was said above, that it is change for the sake of change.

You are right that "icon" would be better than "image" to see what it is good for, but that doesn't mean "image" is wrong. An icon is a special kind of image, so "image" is just less precise, but it's not wrong.

Of course my understanding might be wrong because I'm missing something.

But I'd like to ask you to think over if this PR here is really necessary and helpful, or if it might cause more trouble than it solves, and close this PR if you come to the conclusion that it's really not needed.

Alternatively enlight me and maybe otherse so we can better understand why this PR is needed and not just a nice to have for formal or esthetic reasons.

avatar N6REJ
N6REJ - comment - 18 Sep 2020

our hands are being tied by b/c constraints so we're closing all pr's for now.

avatar N6REJ N6REJ - change - 18 Sep 2020
Status Pending Closed
Closed_Date 0000-00-00 00:00:00 2020-09-18 06:58:05
Closed_By N6REJ
avatar N6REJ N6REJ - close - 18 Sep 2020

Add a Comment

Login with GitHub to post a comment