Skip to content

[4.0] Main menu add new shortcuts#27788

Merged
rdeutz merged 2 commits intojoomla:4.0-devfrom
brianteeman:shpow_new
Feb 11, 2020
Merged

[4.0] Main menu add new shortcuts#27788
rdeutz merged 2 commits intojoomla:4.0-devfrom
brianteeman:shpow_new

Conversation

@brianteeman
Copy link
Copy Markdown
Contributor

The admin menu module has options to show/hide the "add new" shortcuts on the menu. This option was not working. It is now

Partial Pull Request for Issue #26050

image

The admin menu module has options to show/hide the "add new" shortcuts on the menu. This option was not working. It is now
@Quy
Copy link
Copy Markdown
Contributor

Quy commented Feb 3, 2020

On a clean install of your branch, the shortcuts do not appear even though Add New Shortcuts setting is Show.

@brianteeman
Copy link
Copy Markdown
Contributor Author

Just to confirm something can you go to the module and save it - even without changing any settings

@Quy
Copy link
Copy Markdown
Contributor

Quy commented Feb 3, 2020

After saving, they appear. So the value initially is a string and after saving is an integer.

@brianteeman
Copy link
Copy Markdown
Contributor Author

I thought that was the case. The problem is not here with this code. It is a much bigger problem with the installation sql having the wrong values - and not just for this field

}

if ($currentParams->get('menu-quicktask', false))
if ($currentParams->get('menu-quicktask', false) && $this->params->get('shownew', 1) === 1)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if ($currentParams->get('menu-quicktask', false) && $this->params->get('shownew', 1) === 1)
if ($currentParams->get('menu-quicktask', false) && $this->params->get('shownew', 1))

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I see what you are doing here but isnt this just masking the problem that the data in the sql is wrong.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This has nothing to do with SQL as it's not changing the default values. It's just that it changes the explicite check to a simple boolean check. Both ways work exactly the same in this case, the proposed solution just is a bit simpler.

@richard67
Copy link
Copy Markdown
Member

richard67 commented Feb 6, 2020

Is there already an issue or PR for the wrong SQL? Can’t look now, am posting on mobile and will go offline for sleep soon.

@Bakual
Copy link
Copy Markdown
Contributor

Bakual commented Feb 10, 2020

I have tested this item ✅ successfully on 8a9e9ec

This works fine and is basically a no-brainer.

The parameter currently isn't used and this PR adds the corresponding code for it.


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

@jwaisner
Copy link
Copy Markdown
Member

I have tested this item ✅ successfully on 8a9e9ec


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

@jwaisner
Copy link
Copy Markdown
Member

RTC


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

@joomla-cms-bot joomla-cms-bot added the RTC This Pull Request is Ready To Commit label Feb 10, 2020
@Quy
Copy link
Copy Markdown
Contributor

Quy commented Feb 10, 2020

This PR will not display the shortcut icon until Admin Menu module is updated.

@rdeutz rdeutz merged commit 70d27e6 into joomla:4.0-dev Feb 11, 2020
@joomla-cms-bot joomla-cms-bot removed the RTC This Pull Request is Ready To Commit label Feb 11, 2020
@rdeutz rdeutz added this to the Joomla 4.0 milestone Feb 11, 2020
@brianteeman brianteeman deleted the shpow_new branch February 11, 2020 08:36
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.

7 participants