-
-
Notifications
You must be signed in to change notification settings - Fork 825
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
dev/wp#46 - Remove styling that attempts to make inputs match select2 #16882
Conversation
This styling conflicts with CMS styles
(Standard links)
|
Yes, I think this is the best and most maintainable approach. V grateful to @JGaunt for kicking this off though. |
I've done lots of testing on this. The |
This does seem like a good compromise solution. Leaving the styling to the CMS works for this., I've tested on master against WP 5.3 and 5.4 with the same results detailed above. |
Ping @JGaunt I think this is now ok? Would appreciate confirmation? |
Thanks @kcristiano. Which browser/OS did you use to test this? Keen to get some testing on Mac OS and iOS. I've got some demo sites that I set up if that would help with testing. Would also be good to test in Drupal 8, Joomla and Backdrop to see what the impact is on these CMSs. |
I tested in Chromium and Firefox on linux with a WP site. |
In Drupal 7 and 8 with the stock Bartik on its own it's fine. As you've noted it was inconsistent before anyway, so not a blocker, but noting for the record on BEFORE: AFTER: |
@demeritcowboy thanks for doing some testing on this. Yes, your experience with Firefox is consistent with my testing (see above). It is an unfortunate side effect of this PR that the appearance in Drupal is arguably worse. However, I can't see an easy way round this. Also important to note that the functionality isn't affected - it is still clearly a drop-down list and still functions correctly. |
I've done some testing on Drupal 8 now. Results very similar to Drupal 7. I've added the results to the description. I've also done a cursory check in Backdrop. Again very similar to Drupal 7. I haven't been able to get Joomla running in buildkit so haven't tested with Joomla. Help with this welcome. |
On Joomla - Chrome on ubuntu : After If you want a login I do keep a Joomla RC test site running. Ping me in mattermost |
@kcristiano thanks for all the testing! It's interesting that the I feel like this has been extensively tested now. Are we happy for it to be merged? |
@wmortada you are corrrect! I kissed that it was covered. I'll get some more screen shots as soon as I have a chance. I do think this improves WP and is neutral or better on Joomla and Drupal 7. |
Overview
CiviCRM applies some styling that attempts to make
select
elements matchselect2
styling. Unfortunately, this can conflict with the styles applied by the CMS. In particular it causes issues with WordPress 5.3 which applies it's own styling forselect
elements. See https://lab.civicrm.org/dev/wordpress/issues/46.In any case the styling that CiviCRM applies is different from
select2
styling soselect
elements don't look the same.This PR removes CiviCRM's styling so that
select
elements appear as the browser and/or CMS intended. This fixes the issue with WordPress 5.3 and should hopefully reduce the chance of issues in future.Unfortunately, this change does mean that the
select
element loses some of it's styling and rounded corners in other CMSs (i.e. Drupal, Backrop and Joomla) but I can't see an easy way round this problem. As you can see from the testing I've done (see below) the select element is not rendered consistently across platforms in any case.Before
select
in WordPress 5.2 / CiviCRM 5.23:select
in WordPress 5.3 / CiviCRM 5.23:Note that this select is missing an arrow so doesn't look like a select element.
After
select
in WordPress 5.2 / CiviCRM 5.25 plus patch:select
in WordPress 5.3 / CiviCRM 5.25 plus patch:Note the difference in the styling applied by WordPress 5.2 vs 5.3.
Technical Details
The above screenshots were produced on Firefox 74.0 on Ubuntu Linux 18.04
Comments
There is another PR by @JGaunt that attempts to fix the same issue in a different way - #16738
Testing notes
Because the styling is dependent on the operating system, browser and CMS, it should ideally be tested in a full range of platforms. The
select
element will likely look different in each.This means each of the following CMSs:
And the most common operating systems and browsers:
Test results
I've put this into tables because there are a lot of combinations! Blank cells in the grid indicate that it is still to be tested.
For testing purposes I've chosen to look at the 'Mailing Job Status' select field from the Mailings section in Advanced Search.
This is a work in progress. Any help with testing would be much appreciated!
Drupal 7 / CiviCRM 5.25
WordPress 5.2 / CiviCRM 5.26
WordPress 5.4 / CiviCRM 5.26
Drupal 8 / CiviCRM 5.26