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] Components dashboard #27773

Merged
merged 4 commits into from
Feb 16, 2020
Merged

[4.0] Components dashboard #27773

merged 4 commits into from
Feb 16, 2020

Conversation

brianteeman
Copy link
Contributor

@brianteeman brianteeman commented Feb 2, 2020

PR for #27770

Before this PR In the dashboards of joomla 4 we have a situation where sometimes there is an icon and sometimes not.

For all the static preset dashboards the icon was coded in the preset
For the dynamic dashboard for components there was no icon

With this PR then the components module will display icons now. For core components this is done by updating rhe menu table.

Any extension that already defines an icon eg com_pathctester will also work as this is fully backwards compatible

Extension developers can still add either an image or an icon via their components xml

eg
<menu img="class:icon_name">COM_MYCOMPONENT</menu> or
<menu img="pathto/image_name">COM_MYCOMPONENT</menu> or
<menu img="image:pathto/image_name">COM_MYCOMPONENT</menu>

This is a DRAFT. It is ready for testing but I haven't done the install sql or updated the core component xml yet until after feedback.

Documentation Changes Required

Hopefully none as the menu aspect of the xml should already be documented

@Bakual
Copy link
Contributor

Bakual commented Feb 2, 2020

Looks like a good approach to me.
I guess the hardcoded stuff in the menu code can then be removed as well?

@brianteeman
Copy link
Contributor Author

I guess the hardcoded stuff in the menu code can then be removed as well?

Actually not as those components do not have an entry in the _menus table

@Bakual
Copy link
Contributor

Bakual commented Feb 2, 2020

Aww, crap. That's true.
Maybe taking it from the menu preset could work? As I understood those presets there is one for the menu and then several for the dashboards. But I may be wrong, I still don't understand those presets.

@brianteeman
Copy link
Contributor Author

Let's tackle one thing at a time ;)

If you could be so kind as to test what I have done so far then I can finish it

  • sql installs
  • postgresql updates
  • component xml (not used but would be good to be consistent)

Note one thing about my implementation is that it does allow for dashboards to be created when using the alternate template as it uses the same field and formatting for the icon/image (assuming someone creates the dashboard code.

There were many ways to achieve this but I really tried for the most compatible method - not necessarily the cleverest.

@Bakual
Copy link
Contributor

Bakual commented Feb 3, 2020

I have tested this item ✅ successfully on 9b2a716


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

@Bakual
Copy link
Contributor

Bakual commented Feb 3, 2020

I even tried adding img="class:comment" to my component menu declaration and it now shows an icon for it in the "Components Dashboard".
So that imho is a great solution which also works for 3rd parties.

@brianteeman
Copy link
Contributor Author

Thanks @Bakual - I will complete the PR now

@brianteeman
Copy link
Contributor Author

No longer a draft and ready for testing

@brianteeman brianteeman marked this pull request as ready for review February 3, 2020 20:35
@Bakual
Copy link
Contributor

Bakual commented Feb 3, 2020

Just noticed that News Feeds doesn't show the RSS icon you specified. I guess that is no longer (?) part of our FA package?
Or I did something wrong with all the composer/npm stuff

@brianteeman
Copy link
Contributor Author

seems ok to me
https://fontawesome.com/icons/rss?style=solid

@Bakual
Copy link
Contributor

Bakual commented Feb 3, 2020

It may well be that I messed up something in my local installation. If it shows for you and others, then it's fine.

@infograf768
Copy link
Member

Could you post a screenshot of the resulting modification? For example concerning Multilingual Associations.

@brianteeman
Copy link
Contributor Author

I am not aware of any way this impacts on multi lingual associations. But try it and see.

@infograf768
Copy link
Member

It impacts it by adding an icon in the dashboard.
For testers to know what to expect here is a screenshot after patch where the icons are present next to each component title.
Screen Shot 2020-02-08 at 11 30 05

@infograf768
Copy link
Member

conflicts

@brianteeman
Copy link
Contributor Author

It's not an impact it is the desired and intended behaviour as described in the pull request

@infograf768
Copy link
Member

Cool down. By impact, i meant modify and it looks good. (I am not British).
conflicts remain to be solved.

@brianteeman
Copy link
Contributor Author

conflicts resolved

@Bakual
Copy link
Contributor

Bakual commented Feb 9, 2020

@brianteeman I wanted to test your last changes again, and got a very messed up page. To verify it's not on my end, I've did a complet fresh installation and it was still the same. So probably a wrong conflict solving. Can you check if that's the same for you?
"News Feeds" is still not showing the icon - don't know what's wrong here and why it's showing for you and JM (according to his screenshot).
image

@brianteeman
Copy link
Contributor Author

probably a conflict with the changes in #27777

@Bakual
Copy link
Contributor

Bakual commented Feb 9, 2020

Indeed, with this PR you're reverting some of the changes you did there. Eg the <div class="module-wrapper"> is removed again. Probably other changes as well. You need to check that layout again.

@sakiss
Copy link
Contributor

sakiss commented Feb 13, 2020

It breaks the layout in my case:
component_dash_before
After patch
component_dash_after

@brianteeman
Copy link
Contributor Author

@Bakual @sakiss Please can you retest. There was an extra closing div after the merge conflict fix that was breaking the display

@Quy
Copy link
Contributor

Quy commented Feb 13, 2020

Still display issue on a new installation.

27773

@Bakual
Copy link
Contributor

Bakual commented Feb 13, 2020

Still doesn't look right to me.
Have a look at administrator/modules/mod_submenu/tmpl/default.php. If I remember right, you only changed the lines regarding the icon, but now there is a whole bunch of lines changed in the PR.
Still looks like a bad merge conflict solving.

@brianteeman
Copy link
Contributor Author

Sorry for the bad merge conflict - now I really am sure it is correct

@Bakual
Copy link
Contributor

Bakual commented Feb 14, 2020

I have tested this item ✅ successfully on 1684df9

Works perfectly fine again!


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

@Quy
Copy link
Contributor

Quy commented Feb 14, 2020

I have tested this item ✅ successfully on 4aed3ff


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

@Quy
Copy link
Contributor

Quy commented Feb 14, 2020

RTC


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

@joomla-cms-bot joomla-cms-bot added the RTC This Pull Request is Ready To Commit label Feb 14, 2020
@wilsonge wilsonge merged commit 5fac42b into joomla:4.0-dev Feb 16, 2020
@wilsonge
Copy link
Contributor

Thanks!

@joomla-cms-bot joomla-cms-bot removed the RTC This Pull Request is Ready To Commit label Feb 16, 2020
@wilsonge wilsonge added this to the Joomla 4.0 milestone Feb 16, 2020
@brianteeman
Copy link
Contributor Author

thx

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