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

Removes Joomla Sidebar #27252

Merged
merged 8 commits into from
Oct 16, 2023
Merged

Removes Joomla Sidebar #27252

merged 8 commits into from
Oct 16, 2023

Conversation

vingle
Copy link
Contributor

@vingle vingle commented Sep 1, 2023

Overview

As discussed here: https://lab.civicrm.org/dev/joomla/-/issues/45 - Joomla 4 has a sidebar, which defaults open, and means the Civi sidebar squashes the main content more. This PR removes that sidebar.

As Joomla 3 is end-of-life, probably most Joomla users will be on Joomla 4. The sidebar was unique to Joomla, and not present in WordPress or Drupal 9+. Removing the sidebar also stops Civi's main content from being loaded inside a table.

Before

image

After

image

Technical Details

Changes impact two Joomla-specific files – joomla.css and joomla.tpl – so shouldn't impact other CMSs. Joomla 3 is End-of-life since August 17, other than the paid Extended Support Release programme, and this hasn't been tested there.

Comments

If Joomla 3 support is required, this needs testing on Joomla 3.

CSS fix for stripping sidebar
Removes old markup specific to the sidebar, and fixes padding.
Removes extra </div>
@civibot
Copy link

civibot bot commented Sep 1, 2023

🤖 Thank you for contributing to CiviCRM! ❤️ We will need to test and review this PR. 👷

Introduction for new contributors...
  • If this is your first PR, an admin will greenlight automated testing with the command ok to test or add to whitelist.
  • A series of tests will automatically run. You can see the results at the bottom of this page (if there are any problems, it will include a link to see what went wrong).
  • A demo site will be built where anyone can try out a version of CiviCRM that includes your changes.
  • If this process needs to be repeated, an admin will issue the command test this please to rerun tests and build a new demo site.
  • Before this PR can be merged, it needs to be reviewed. Please keep in mind that reviewers are volunteers, and their response time can vary from a few hours to a few weeks depending on their availability and their knowledge of this particular part of CiviCRM.
  • A great way to speed up this process is to "trade reviews" with someone - find an open PR that you feel able to review, and leave a comment like "I'm reviewing this now, could you please review mine?" (include a link to yours). You don't have to wait for a response to get started (and you don't have to stop at one!) the more you review, the faster this process goes for everyone 😄
  • To ensure that you are credited properly in the final release notes, please add yourself to contributor-key.yml
  • For more information about contributing, see CONTRIBUTING.md.
Quick links for reviewers...

➡️ Online demo of this PR 🔗

@civibot civibot bot added the master label Sep 1, 2023
@colemanw
Copy link
Member

colemanw commented Sep 1, 2023

Thanks @vingle! It would be great if you could fix the linting errors caught here and an added bonus if you could squash the commits down to one.
image

@vingle
Copy link
Contributor Author

vingle commented Sep 1, 2023

@colemanw thanks - will do. Am keen to clean up joomla.css completely, but that's probably a separate PR.

@seamuslee001
Copy link
Contributor

I think in some drupal themes like Garland it could have its own side bar but yes. I think if we are doing this and completely gutting the sidebar can I suggest we get rid of https://github.com/civicrm/civicrm-core/blob/master/CRM/Core/Invoke.php#L216 and https://github.com/civicrm/civicrm-core/blob/master/CRM/Core/Joomla.php#L32 and https://github.com/civicrm/civicrm-core/blob/master/templates/CRM/Block/blocks.tpl.

Just also noting that JMA currently has 2 clients that still use Joomla 3 for mainly joomla plugin / module reasons

@vingle
Copy link
Contributor Author

vingle commented Sep 2, 2023

I think in some drupal themes like Garland it could have its own side bar

That's true. I think it's D8 onwards it's no longer seen, have changed.

can I suggest we get rid of…

That would be a big improvement to the PR – but it's not something I feel confident to do. (removing the function from /core/Joomla.php seems to make that file unnecessary too?)

JMA currently has 2 clients that still use Joomla 3

Is it possible to get a sense if they value the sidebar? There hasn't been any pushback to removing it on the Gitlab issue but it's maybe not been seen by so many Joomla users.

@seamuslee001
Copy link
Contributor

@vingle I'll see if I can get someone to check in with them.

Fixes linting errors, including missing semicolon & makes line-space consistent
@vingle
Copy link
Contributor Author

vingle commented Sep 4, 2023

@colemanw – I've cleaned up the css spacing/bug, but struggled to squash - either thru the desktop or webUI (because one commit is a merge?). If you can squash on merge, that's appreciated - or I can make a new PR?

Not sure about implementing Seamus's suggestions as I don't know how best to check if it would impact other things than Joomla's UI beyond lots of testing on the different CMSs.

@andrewpthompson
Copy link
Contributor

@vingle I did a quick test on Joomla 3 (as requested on Mattermost).

No major problems but I did notice 2 minor things:

  1. The font size of some text is smaller after applying the patch (like the text below the Grants Summary heading in the screenshot below).
  2. There is no white space at all on the left and right side between elements and the edge of the browser window. On the right side this means the browser's right scrollbar can overlap some elements.

image

@vingle
Copy link
Contributor Author

vingle commented Sep 13, 2023

@andrewpthompson thanks alot for doing the check. I foolishly haven't left any J3+Civi dev spaces to test against, but will set one up, fix and resubmit. The shrinking font-size is a little unexpected.

@JoeMurray
Copy link
Contributor

@seamuslee001 could you also do a runtime test and include testing of your suggested code improvements to patch then get @monishdeb to QA. @Edzelopez could you check with one or both of our J3 clients?

@vingle
Copy link
Contributor Author

vingle commented Sep 24, 2023

@andrewpthompson - I've just setup a J3 and Civi to test and while I can't replicate the lack of gap on the edges after refreshing the browser (see below), I can see the font-size shrinks from the already too small 13px to 11px.

This is because sizes are set in ems not rems (like much of Civi css sadly) so removing the table from the markup has changed the parent font-size definition for .description from the 13px of .crm-container table in civicrm.css to the 11px of #crm-container in joomla.css. A quick fix for this is to remove the #crm-container selector in Joomla.css - that should restore the size to how it looked before.

image

Tested on Joomla 3/4, to fix issue raised here #27252 (comment)
@colemanw
Copy link
Member

@vingle... really appreciate your work on this!
It appears that all reviewer feedback has been addressed, so 🚀

@colemanw colemanw added the merge ready PR will be merged after a few days if there are no objections label Oct 14, 2023
@vingle
Copy link
Contributor Author

vingle commented Oct 15, 2023

Thanks @colemanw. I can now make my radical clean-up joomla.css PR: #27834!

@andrewpthompson
Copy link
Contributor

Thanks for your work on this @vingle
I think the other problem I saw was some custom css of mine that I added a very long time ago and had forgotten about but thanks for checking and fixing the font size issue. Nice work.

@mattwire mattwire merged commit a9e0f90 into civicrm:master Oct 16, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
master merge ready PR will be merged after a few days if there are no objections
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants