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-480: Fix broken buttons #492

Conversation

reneolivo
Copy link
Collaborator

@reneolivo reneolivo commented Mar 9, 2021

Overview

This PR fixes an issue where CiviCRM buttons inside of Bootstrap templates break.

Before & After

CRM buttons

Before

pr

After

pr

Technical details

All .crm-button and .crm-form-submit buttons inside of #bootstrap-theme would have their style overridden by a color: inherit f3e73be#diff-293f309d5b967b2ce90466201daefdeaad764020a635e624f1ac6b19d225fb2bR257 and text-transform: none f3e73be#diff-293f309d5b967b2ce90466201daefdeaad764020a635e624f1ac6b19d225fb2bR279 that should be applied to bootstrap buttons. To avoid this, we overrode the definitions for .crm-button and .crm-form-submit by adding an !important to their color and text transform properties. This way Bootstrap can't replace these properties for these buttons.

Upload buttons

Before

pr

After

pr

Technical details

Upload buttons are submit buttons used in forms, not necessarily for uploading files. These need to look like any other button. Previously we defined these upload buttons as outlines:
e948807#diff-d70c310903cd4caf4066146aa6a16294bb25f009a7f6a7f7b85d63c494525f3dR1-R7
The reason for this is not clear (since the file history is not complete) and the // Makes next and upload buttons look like primary buttons: comment is not clear since button-outline-variant displays outlined buttons, not primary buttons. We have only removed this from the upload buttons, but need to investigate "next" buttons to confirm if the style is right for them.

Backstop Report:

https://github.com/compucorp/backstopjs-config/actions/runs/636368203

Some artifacts were found, but manually confirmed that no actual regressions were introduced.

@deb1990
Copy link
Contributor

deb1990 commented Mar 9, 2021

@reneolivo In general looks good, but cant we avoid !important?
Also please check the Backstop Report before merging.
And as a process, please attach the backstop task link in the comments.

@reneolivo reneolivo changed the title CPS-332: Fix broken buttons CPS-480: Fix broken buttons Mar 9, 2021
@reneolivo
Copy link
Collaborator Author

@deb1990 thanks for the feedback. I have removed the !important statements by defining the rules using the #bootstrap-theme selector. It makes sense since we need to target CRM buttons being used inside of Bootstrap templates.

Also added the missing spacing between buttons. This was also being overridden by Bootstrap.

Finally, added the Backstop report link to the description.

@reneolivo reneolivo force-pushed the CPS-332-support-new-button-markup branch from d55e063 to ef0a299 Compare March 9, 2021 21:16
@reneolivo reneolivo changed the base branch from master to CPS-427-civicase-civicrm-5.35-alignment March 16, 2021 19:31
@reneolivo reneolivo merged commit 225dfa3 into CPS-427-civicase-civicrm-5.35-alignment Mar 16, 2021
@reneolivo reneolivo deleted the CPS-332-support-new-button-markup branch March 16, 2021 19:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants