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

CPS-322: Support new Button markup #473

Merged
merged 3 commits into from
Nov 5, 2020

Conversation

deb1990
Copy link
Contributor

@deb1990 deb1990 commented Sep 29, 2020

Overview

Recently in the Civicrm Core(civicrm/civicrm-core#18410), all the input type button markups has been changed to button elements. This caused a lot of styles to break when using Shoreditch theme. So this PR fixes those UI related style issues.

Before (Only showing some examples)

2020-10-19 at 3 08 PM

2020-10-19 at 3 09 PM

2020-10-19 at 3 10 PM

After (Only showing some examples)

2020-10-19 at 3 12 PM

2020-10-19 at 3 12 PM

2020-10-19 at 3 11 PM

Technical Details

  1. Changed css selector from input to button in
scss/civicrm/administer/contribute/_managepremiums.scss
scss/civicrm/common/_buttons.scss
scss/civicrm/contact/pages/_contributions.scss
scss/civicrm/financial/_batchtransaction.scss
scss/civicrm/search/pages/_full-text-search.scss
  1. Made adjustments to the following files to support new markup
// There are no extra parent divs for buttons, so moved the css one level up
scss/bootstrap/overrides/crm/_button.scss 

// no extra css required for .crm-i, as items inside buttons are auto aligned
scss/civicrm/administer/display/_display.scss 

// styles for .crm-i removed, as items inside buttons are auto aligned, and crm-i-button does not exist anymore
scss/civicrm/search/pages/_advanced-search.scss 
  1. Added styles specific to Case Types page in scss/civicrm/administer/civi-case/_case-types.scss. And this code was moved from scss/civicrm/administer/common/_common.scss .
    This style was moved to the common.scss in HW-382: Styled Individual casetype setting page #49. But now it is causing regressions in other pages. And other pages are working fine without these changes, so made it only for case type page.

Comments

Backstop JS and manual test suites were run to ensure no regressions are present.

@deb1990 deb1990 force-pushed the CPS-332-support-new-button-markup branch 2 times, most recently from d765d11 to e8c03db Compare October 2, 2020 06:36
@eileenmcnaughton
Copy link
Contributor

Just noting I tried this & it fixed the issue I see on 5.31 at

http://dmaster.local/civicrm/admin/uf/group/update?action=update&id=1&context=group

Screen Shot 2020-10-08 at 8 58 56 AM

vs

Screen Shot 2020-10-08 at 9 00 37 AM

@deb1990
Copy link
Contributor Author

deb1990 commented Oct 15, 2020

@eileenmcnaughton Thanks for checking. PR should be ready within the next week.

@deb1990 deb1990 force-pushed the CPS-332-support-new-button-markup branch from 308c999 to af736e0 Compare October 19, 2020 09:59
@deb1990 deb1990 changed the title (WIP) CPS-322: Support new button markup CPS-322: Support new Button markup Oct 19, 2020
scss/civicrm/administer/display/_display.scss Show resolved Hide resolved
scss/civicrm/common/_buttons.scss Show resolved Hide resolved
scss/civicrm/contact/pages/_contributions.scss Outdated Show resolved Hide resolved
@@ -23,7 +23,7 @@
}

.form-layout-compressed {
input.crm-form-submit {
button.crm-form-submit {
margin-left: 5px;
padding: 4px 10px;
Copy link
Collaborator

Choose a reason for hiding this comment

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

In _contributions.scss we changed the padding values from 4px 10px to 8px 11px. Why wasn't the same change applied here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch. the previous one also should be 4px 10px. Not sure why I made it like that.

@extend .btn-sm;
margin-left: 20px !important;
margin-top: -8px;
Copy link
Collaborator

Choose a reason for hiding this comment

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

are we trying to fix a new issue or an existent one? changing from inputs to button misaligned the element?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes the position was changed after changing to button. Because we have a long tree of styles being applied from different themes/css file(reset.css, civicrm.css etc).

But I found a better way of fixing this, that is by adding

float: none;
vertical-align: initial;

Copy link
Collaborator

@reneolivo reneolivo left a comment

Choose a reason for hiding this comment

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

@deb1990 also, why did the package-lock.json file change?

@deb1990
Copy link
Contributor Author

deb1990 commented Oct 20, 2020

@deb1990 also, why did the package-lock.json file change?

I never knew how this works, but now I have a answer https://stackoverflow.com/a/50868741.
The package-lock.json is supposed to be updated every time you run npm install. Otherwise we need to run npm ci to download exact versions. So in development its fine to use npm install and in production npm ci should be used.

@reneolivo
Copy link
Collaborator

@deb1990 can you please use nvm use and then npm install ? the package-lock.json is not supposed to change when installing local packages. The only reason for changing is when installing using a different Node version than the one enforced by NVM/NPM.

@eileenmcnaughton
Copy link
Contributor

5.31 drops tomorrow so you probably want to merge this & tag a 5.31 compatible release

@deb1990
Copy link
Contributor Author

deb1990 commented Nov 5, 2020

@eileenmcnaughton Thanks for the reminder. It will be merged today.

@deb1990 deb1990 merged commit 5bf1577 into master Nov 5, 2020
@deb1990 deb1990 deleted the CPS-332-support-new-button-markup branch November 5, 2020 15:18
@deb1990
Copy link
Contributor Author

deb1990 commented Nov 5, 2020

@eileenmcnaughton This is the release to support CiviCRM 5.31 buttons.
https://github.com/civicrm/org.civicrm.shoreditch/releases/tag/1.0.0-beta.4

@eileenmcnaughton
Copy link
Contributor

yay

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants