civicrm / org.civicrm.shoreditch

Other
45 stars 59 forks source link

MAE-493: Style the form buttons and Direct Debit batch form actions #496

Closed swastikpareek closed 3 years ago

swastikpareek commented 3 years ago

Overview

In the latest CiviCRM update(5.35), there were new button classes were updated and hence introduced some UI impairments on the Direct Debit Batch pages. This PR fixes that.

Before

Screenshot 2021-03-16 at 12 24 11 PM

After

Screenshot 2021-03-15 at 7 49 43 PM

Technical Details

Tests

Link to the Github action workflow for running backstop tests - https://github.com/compucorp/backstopjs-config/actions/runs/657199439

List of failed scenario

deb1990 commented 3 years ago

Can we fix the following please. 2021-03-17 at 12 17 PM

Also are you sure these are false alarms? It might be a regression because of changed button styles, please double check and confirm :)

  1. Search builder
  2. Search Builder - Search result

Also this PR fixed a issue in Search Results - Add to Group Modal - more actions 2021-03-17 at 12 20 PM But can we add a bit of margin bottom? So that it looks perfect? It might be related to the first problem I mentioned.

swastikpareek commented 3 years ago

@deb1990 :

Can we fix the following please. Don't think we should fix this as this is not in the initial CiviCRM (5.28.3) implementation. Please check it here

Screenshot 2021-03-17 at 12 27 09 PM

Same for Search Results - Add to Group Modal - more actions

Also are you sure these are false alarms? It might be a regression because of changed button styles, please double check and confirm :) This is correct that the changes done in this PR affects the styling of the search results, but this is not a regression, as these pages are already not matching the old version(5.28.3 version) even on the 1.0.0-beta.4 version of shoreditch. The reason for this is because in the new CiviCRM, the <input> has been changed to <button/> tag and the button has more padding/line-height and font-size assigned to it by the user agent stylesheet and already shoreditch button styles. See the gif. This compares the version 1.0.0-beta.4 (5.35) with (5.28.3) spacing This means the PR fixes some of the styles of the button but not all. It fixes the padding but not the font size and line-height.

Since styling the buttons are out of the scope of this ticket and also, they don't look broken and looks a bit more spaced. I think it's fine.

deb1990 commented 3 years ago

Okay @swastikpareek , ideally we should fix the things completely when touching them, otherwise we again have to come back to these pages to fix the same thing. But since this is related to CiviCRM 5.35 update, and I know its urgent, I am approving it. But please create a ticket in the Backlog to fix these later for the following issues

https://user-images.githubusercontent.com/5058867/111426577-dce7a480-871a-11eb-8d37-07a5aba4c3a8.png https://user-images.githubusercontent.com/5058867/111426881-4cf62a80-871b-11eb-9636-d5c48271762b.png

jamienovick commented 3 years ago

Hi @deb1990 @swastikpareek - what's the reason for not making them look good now?

swastikpareek commented 3 years ago

Hi @deb1990 @swastikpareek - what's the reason for not making them look good now?

@jamienovick : If you are talking about the batch screen buttons https://user-images.githubusercontent.com/5058867/111426577-dce7a480-871a-11eb-8d37-07a5aba4c3a8.png https://user-images.githubusercontent.com/5058867/111426881-4cf62a80-871b-11eb-9636-d5c48271762b.png

The reason we are not adding spacing is that this is the default appearance of these buttons on CiviCRM-5.28.3. And if we try to add more spacing, there might be implications for it and can cause regressions. Also, fixing spacing is not part of the main ticket, the idea was to match it with the old civi implementation, and the estimates and work log was done while considering this fact.