Improve datePicker placeholder/icons #15848
Conversation
|
(Standard links)
|
|
@agh1 this one is pretty straightforward to review. Shouldn't impact existing screens but improves flexibility with an eye toward form-builder. |
There was a problem hiding this comment.
-
General standards
-
(
r-explain) PASS -
(
r-user) PASS: it should be unnoticeable -
(
r-doc) PASS: however, it would be nice to have some notes in the developer docs saying you can give a field the attributeplaceholderortime-placeholderto specify a different placeholder -
(
r-run) ISSUE: it appears that the input lacks thecrm-placeholder-iconclass which the CSS selects for and instead has the classcrm-placeholder-right. That results in the placeholder being the wrong font and not aligned to the right. I suspect that you changed your mind about what it should be called, and as long as the element and the CSS have the same class you'll be fine.New event - WordPress - Firefox -
master:
New event - WordPress - Firefox - with this merged:
-
-
Developer standards
| .crm-container input.dateplugin::placeholder, | ||
| .crm-container input.crm-form-date::placeholder, | ||
| .crm-container input.crm-form-time::placeholder { | ||
| .crm-container input.crm-placeholder-icon::placeholder { |
There was a problem hiding this comment.
I think this should be crm-placeholder-right here and below.
| .crm-container input.crm-placeholder-icon::placeholder { | |
| .crm-container input.crm-placeholder-right::placeholder { |
Also, are there three separate blocks because IE needs it to be that way?
There was a problem hiding this comment.
According to what I've read, all the major browsers support ::placeholder except MS is still lagging behind, and also inconsistent about which one of the two brand-prefixes it wants.
js/crm.datepicker.js
Outdated
| .change(updateDataField) | ||
| .timeEntry({ | ||
| spinnerImage: '', | ||
| show24Hours: settings.time === true || settings.time === undefined ? CRM.config.timeIs24Hr : settings.time == '24' | ||
| }); | ||
| if (!placeholder) { | ||
| $timeField.addClass('crm-placeholder-right'); |
There was a problem hiding this comment.
You could instead switch this (and the spot on line 82) to crm-placeholder-icon to match the CSS, but I like having -right in the class to be clear that we're aligning it to the right. I suppose if you want to be real fancy you could set two classes: crm-placeholder-icon that sets it to be Font Awesome and crm-placeholder-right that aligns the placeholder to the right.
|
Thanks for the review @agh1. Good catch, I did change my mind mid-commit because I think |
Overview
Small tweaks to the datepicker widget to improve flexibility & consistency.
Before
Inconsistent handling of the placeholder properties. FontAwesome font sometimes applied inappropriately. Time icon was applied conditionally but date icon would clobber any other icon specified.
After
Added a css class
crm-placeholder-iconto abstract the idea of an input with a font icon as a placeholder. Apply that class only if we're using an icon and not some other placeholder.