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] Fixed titles for 3rd party component dashboards #31382

Merged
merged 1 commit into from
Nov 14, 2020

Conversation

Harmageddon
Copy link
Contributor

Summary of Changes

In version 4, extensions can create their own dashboards. While this is a really cool feature, it seems to be mostly undocumented, so I had to do a bit of research on how to use this, by reading PRs like #28027, #27773 and similar.
The code for com_cpanel contains parts to define the title and icon used for the dashboard. However, these don't seem to work. There is a test that checks if the $extension string starts with com_. But previous filtering converts com_something into com-something.

Testing Instructions

  1. Install a component that uses an own dashboard. For this purpose, I created https://github.com/Harmageddon/com_demoj4 which you can use.
    If you develop your own extension, this is the perfect occasion to test dashboards. ;-)
  2. In backend, click on the dashboard icon next to the component's menu item.
  3. Look at the title on top of the page.

Actual result BEFORE applying this Pull Request

There is no icon and the title says "Home Dashboard".

Expected result AFTER applying this Pull Request

An icon is shown and the title says "DemoJ4 Dashboard" (or what you have specified as COM_<YOUR COMPONENT>_DASHBOARD_TITLE.

Documentation Changes Required

The whole feature lacks documentation. I'm not even sure if it is correct to name the dashboard com_demoj4 for a component with the name com_demoj4 or it should be just demoj4.

Pinging @Bakual as I saw you used the dashboard already for your extension. Of course, it would be great to have other extension developers here as well who might be already using this feature as well.

@Bakual
Copy link
Contributor

Bakual commented Nov 11, 2020

I wasn't able to translate the string yet to my liking, I must be missing something somewhere. But code review looks fine. There is no reason to use stringURLSafe for the $extension variable. And a few lines later (#64) the same stringURLSafe is done again on the $extension. So current code pretty sure smells.

@Harmageddon
Copy link
Contributor Author

Harmageddon commented Nov 11, 2020

Is your dashboard identifier "myextension" or "com_myextension"? I looked at the code of com_sermonspeaker you uploaded for another PR, where it was only "sermonspeaker". ;-)
As I said, I'm not sure whether it has to be "com_myextension", but if we omit the "com", I don't know how to load the appropriate language files.


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

@ChristineWk
Copy link

I have tested this item ✅ successfully on c117565

OK, confirm Expected Result and "nice" icon :-)
But I didn't check/know about the language matters.


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

@ceford
Copy link
Contributor

ceford commented Nov 12, 2020

I have tested this item ✅ successfully on c117565

I did notice that the Components Dashboard is missing an icon alongside J4 Demonstration Component, the dasboard icon is misaligned and has title="%s Dashboard" - but all of that is related to the demo component. I may look into that to see if I can help with some documentation.


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

@ChristineWk
Copy link

I did notice that the Components Dashboard is missing an icon alongside J4 Demonstration Component,

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

All other Components do also not have an icon alongside. So, I don't understand (?) Tks.

@ceford
Copy link
Contributor

ceford commented Nov 12, 2020

Fix for test script:
<menu img="class:bullhorn">
shows the icon in the components dashboard.


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

@Bakual
Copy link
Contributor

Bakual commented Nov 12, 2020

Is your dashboard identifier "myextension" or "com_myextension"? I looked at the code of com_sermonspeaker you uploaded for another PR, where it was only "sermonspeaker". ;-)
As I said, I'm not sure whether it has to be "com_myextension", but if we omit the "com", I don't know how to load the appropriate language files.

Yep, that looks indeed fishy in the code. Since the core dashboards all use just "content" or "users", one should expect 3rd parties can use the same naming convention. But those names all come from the com_cpanel.ini which is architecturally wrong. They should come from com_content.ini or com_users.ini which would also make it work for 3rd parties then.
I'll try to see if I can find a solution which don't has a special case for core extensions

@Bakual
Copy link
Contributor

Bakual commented Nov 12, 2020

I've fixed the titles with #31392. My PR already also incorporates this one for better testing. I'll update mine as soon as this here is merged.

@Quy
Copy link
Contributor

Quy commented Nov 12, 2020

RTC


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

@joomla-cms-bot joomla-cms-bot added the RTC This Pull Request is Ready To Commit label Nov 12, 2020
@richard67
Copy link
Member

I've fixed the titles with #31392. My PR already also incorporates this one for better testing. I'll update mine as soon as this here is merged.

#31392 meanwhile has been replaced by #31397 .

@richard67 richard67 merged commit 0c2e691 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
@richard67
Copy link
Member

@Bakual I've updated your PR #31397 by solving the "conflict".

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.

None yet

7 participants