The implementation of our element alteration in Commerce Discount Date is quite poor. We're setting a title for the start date but not making it visible. We're hiding elements and implementing UI anti-patterns via CSS when we should just be altering elements to be non-#access-ible. This needs a complete re-implementation.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

rszrama created an issue. See original summary.

rszrama’s picture

Status: Active » Needs review
FileSize
16.38 KB

Go, go, gadget testbot!

Status: Needs review » Needs work

The last submitted patch, 2: 2600040-2.discount_date_styles.patch, failed testing.

rszrama’s picture

Bah, apparently the Date module is very touchy when it comes to the start_todate element. I'm sure there's a fix, but it doesn't lie in hiding the element via #access or a simple conversion of the element to a value unfortunately.

joelpittet’s picture

Status: Needs work » Needs review
Issue tags: +Needs screenshots
FileSize
8.81 KB

Yeah this is a pain. Took quite some time to wrangle this into shape but I think I got something...

Will take screenshots if it passes.

joelpittet’s picture

Issue summary: View changes
FileSize
20.07 KB
17.07 KB

Here's a before and after in my admin theme (adminimal)


joelpittet’s picture

Issue summary: View changes
Issue tags: -Needs screenshots
FileSize
24.24 KB
18.62 KB

I removed the fieldset styling as I figure the admin theme can do a good job at that. My goal is to remove as much custom styling as possible.

Seven Before

Seven After:

joelpittet’s picture

I can add some better date icons back if you want, but those ones were pretty gross. And inline the labels still if you want. But would rather remove styles than add them in this case.

nvahalik’s picture

Just adding a note that this patch will end up causing a conflict with the date/usage merge patch.

Status: Needs review » Needs work

The last submitted patch, 5: stop_hiding_via_css-2600040-5.patch, failed testing.

nvahalik’s picture

Status: Needs work » Needs review
FileSize
8.77 KB
882 bytes

Reroll of the patch. Adding vertical tabs also introduced a slight regression where the first field date text field was 5px lower than the second one. So this patch also fixes that by removing the extra 5px of padding.

  • joelpittet committed f78bf1b on 7.x-1.x
    Issue #2600040 by joelpittet, nvahalik, rszrama: Stop hiding via CSS...
joelpittet’s picture

Status: Needs review » Fixed

Thanks @nvahalik I've committed this with a calendar icon replaced with SVG from http://evil-icons.io/.

We can change the icon if there is a decision made #2625938: Replace image icons with SVG for HiDPI devices for commerce but I feel that is better than what we had.

joelpittet’s picture

For the visual minded: here's a screenshot.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.