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

dev/user-interface#35 civicrm.css: add support for mobile devices on forms #19968

Merged
merged 1 commit into from
Apr 6, 2021

Conversation

mlutfy
Copy link
Member

@mlutfy mlutfy commented Apr 5, 2021

Overview

CSS tweak so that CiviCRM forms are less hostile to use on mobile devices (https://en.wikipedia.org/wiki/Mobile_device).

https://lab.civicrm.org/dev/user-interface/-/issues/35

Before

mobile-before-2021-04-05_09-42

After

mobile-after-2021-04-05_09-43

Technical Details

Added media queries (https://developer.mozilla.org/en-US/docs/Web/CSS/Media_Queries/Using_media_queries).

The impact should be minor, assuming 99.99999% people override this on their custom CSS.

Comments

The above screenshots are from the WordPress wpmaster test site (https://wpmaster.demo.civicrm.org/contribution-page/), which uses the "twentythirteen" theme. The default Drupal7 theme does not support mobile devices (I guess mobile was fairly new in 2011, with the arrival of 4G), but Drupal9 seems cutting edge fortunately does (https://d9-master.demo.civicrm.org/civicrm/contribute/transact?id=1).

@civibot
Copy link

civibot bot commented Apr 5, 2021

(Standard links)

@civibot civibot bot added the master label Apr 5, 2021
@mlutfy mlutfy changed the title civicrm.css: add support for mobile devices on forms dev/user-interface#35 civicrm.css: add support for mobile devices on forms Apr 5, 2021
@eileenmcnaughton
Copy link
Contributor

@vingle @artfulrobot any opinions?

@vingle
Copy link
Contributor

vingle commented Apr 5, 2021

Thumbs up from me. It's a faff creating local hacks for every site.

Something that's held me back from making a PR in the past is civicrm.css hasn't until now had any breakpoints set. So while I'd definitely support min-width: 480px as the smallest breakpoint size as that's what UIKit uses – and I think it's also used in places by WordPress – I'm not sure if/how it's used elsewhere in Civi. ie Bootstrap v5 & v4 use 576px as it's xs/smallest media query– while v3.4 uses a (really-wide) 768px.

That said, it still seems a good improvement. It'll be easier for someone to create an override specific to their front-end theme's breakpoint as they can just tweak the media query, and some figure for width has to be used.

@eileenmcnaughton eileenmcnaughton added the merge ready PR will be merged after a few days if there are no objections label Apr 5, 2021
@eileenmcnaughton
Copy link
Contributor

Thanks @vingle - I've added merge ready based on your response. If nothing else comes up in the next couple of days we can merge

@colemanw
Copy link
Member

colemanw commented Apr 6, 2021

Not a blocker, but I just want to note that different themes have different breakpoints, and I've already made headway on accommodating those differences during the KAM/SmartMenus project. For the menubar css, breakpoint variables are defined server-side according to CMS and then variable placeholders in the css file get replaced by asset-builder.

I think we should reuse and keep building on that system when introducing other breakpoint queries into our stylesheets.

@artfulrobot
Copy link
Contributor

Looks like it solves an immediate problem for mobile, so good from me.

(almost a shame to choose to maintain floats for desktop/non-mobile, but understand the need to keep this PR focussed)

@mlutfy
Copy link
Member Author

mlutfy commented Apr 6, 2021

@colemanw Would you be OK with me adding a "todo" comment in the code, so that if this opens a can of worms for people who want to tweak it, they will know where to look?

I had a quick look at the renderMenubarStylesheet function, it looks very nice, but I have to admit I'm not sure how to handle it (between the hook usage and would need to refactor a bit, but definitely seems like the best way forward).

@colemanw
Copy link
Member

colemanw commented Apr 6, 2021

That's fine.

@mlutfy
Copy link
Member Author

mlutfy commented Apr 6, 2021

👍 done

thanks @vingle @artfulrobot @colemanw for the feedback.

@colemanw colemanw merged commit 65afdc7 into civicrm:master Apr 6, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
master merge ready PR will be merged after a few days if there are no objections
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants