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

CIVIMM-249: UI Improvements on compuclient 7.x-4.0 (CiviCRM - 5.75) upgrade #567

Open
wants to merge 11 commits into
base: master
Choose a base branch
from

Conversation

swastikpareek
Copy link
Collaborator

@swastikpareek swastikpareek commented Dec 25, 2024

Overview

This PR adds basic CSS to improve UI styles of the CiviCRM pages after CiviCRM - 5.75 raised by Jamie on https://compucorp.atlassian.net/wiki/spaces/CN/pages/4682776583/Compuclient+4.0+upgrade+-+UI+Shoreditch+issues document.

Before/After

Component Before After
Top Margin fixture on forms Form Vertical spacing - before Form vertical spacing - after
Contribution Page Contribution page - before Contribution page - after
Accordion styles Accordion 1 - before Accordion 2 - before Accordion 1 - after Accordion 2 - after
Vertical padding on the help box Vertical Spacing  - before Vertical Spacing - after
Custom FieldSet styles Custom fieldset accordion - before Custom fieldset accordion - after
UI Modal field vertical spacing UI dialog box inner form item spacing - before UI dialoig box accordion alignment - before UI dialog box inner form item spacing - after UI dialoig box accordion alignment - after
Create Discount modal accordion label styles Discount form accordion header - before Discount form accordion header - after

Technical Details

Most changes were done using plain CSS adjustments. The accordion header styles were taken (and improved) from #565 since that PR was very old, reviewing and expecting the author to update the changes will be time-consuming, so we decided to copy the changes in our PR.

Backstop JS Report

I have tried running the backstop test suite, but some screens of the backstop test suite seem incompatible with the new Civic screens and therefore, I didn't bother to run the whole suite and just ran basic scenario groups - like events, contributions, contacts and administration screens.
All the failed scenarios were related to the spacing and restyling of accordions we did.

@swastikpareek swastikpareek force-pushed the CIVIMM-249-ui-improvements-after-civi-5.75-upgrade branch from d02cb1d to 59b346e Compare December 25, 2024 14:11
@swastikpareek swastikpareek force-pushed the CIVIMM-249-ui-improvements-after-civi-5.75-upgrade branch from 59b346e to 062b949 Compare December 25, 2024 14:13
@swastikpareek swastikpareek force-pushed the CIVIMM-249-ui-improvements-after-civi-5.75-upgrade branch from 062b949 to 547e9e9 Compare December 25, 2024 14:16
@@ -1,3 +1,4 @@
//stylelint-disable selector-max-compound-selectors, max-nesting-depth, selector-max-id

Choose a reason for hiding this comment

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

@swastikpareek Question do we need to disable these selectors? if now can we remove these stylints?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Moving ahead, we will be planning to get rid of this theme and styles anyways, as the CiviCRM is doing a branch new version https://civicrm.org/blog/josh/announcing-civicrm-6 CIVICRM 6 and we will use the new theme framework riverlea to base our new shoreditch-theme (at least that's the plan for now as per my discussion with @jamienovick)

So, I suggest that we only touch the CSS files we need to fix. Also, removing selector-max-compound-selectors, max-nesting-depth, selector-max-id stylelint rules from the global configuration can open the doors for other developers to go crazy and write any CSS selector they want. And we shouldn't allow that, if we want to disable specific rules for specific files, we can disable them as per the files.

Hope that answers your question.

Choose a reason for hiding this comment

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

Got it @swastikpareek Thanks

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.

2 participants