Add vertical tabs to the discount configuration form

+1 to this idea, it would make the UI a bit less "custom" which would make it easier to keep to the style of the themes that are styling it.

This idea also gets a +1 from me, it'd make that entire user interface so much.. usable. Tagging for commerce-sprint.

@rszrama if you could post even a broken patch that kickstarts this, I'll take this an run with it.

@joelpittet - Attempting to get this started with a patch.

One issue with this patch: I needed to modify the css to prevent some floating problems, but did not find a scss config file within the repo, so I only modified the css files, not the related scss.

Any feedback is appreciated. I'm happy to re-roll with another approach and with proper scss usage.

There's a patch so I'm setting to Needs Review.

hey @daggerhart

I'm not sure what you mean by scss config file, but the scss file is just in scss/. I updated the patch with the code moved into the scss file and also changed the $k variable to something clearer, drupal standard isn't big on naming stuff like that unless maybe it is just a counter.

+++ b/commerce_discount.module
@@ -322,6 +322,27 @@ function commerce_discount_form_commerce_discount_form_alter(&$form, $form_state
+  $additional_settings = array(
+    'commerce_compatibility_strategy',
+    'commerce_compatibility_selection',
+    'discount_usage_per_person',
+    'discount_usage_limit',
+    'commerce_discount_date'
+  );

Do you think this should be moved into a config or a define or is there somewhere else we can pull this from? Seems like a bad place to have the additional setting types defined.

Hi @smccabe,

Thanks for the feedback on the patch.

I found the scss files, and the note in the README with compiling instructions, but didn't like the results of my compile. It replaced every single line in the file. Not sure if the scss file is out of date or if it was an OS line ending issue, so I haven't messed with it further. I'd like for someone more familiar with the scss file than I am to verify that it is up to date and safe to use for css compilation.

You're right about it not being good to use a hard-coded list of fields for the vertical tabs, so I've attached a patch that takes a more flexible approach. It loops through all visible child fields of commerce_discount_fields and ignores a hard-coded list of fields we don't want in the vertical tabs.

This patch fixed one of the css float issue by adding the clearfix class to the commerce_discount_offer field, but does not attempt to change any css.

Let me know what you think.

hey @daggerhart

I like the dynamic setup better, I just added a small comment to explain why we were skipping those options. I also put the CSS in the patch since I got the SCSS stuff working correctly. The below worked for me and produced only the relevant changes to the CSS.

sass scss/commerce_discount.scss:css/commerce_discount.css --style expanded

Hi @smccabe,

That css build looks much better, and the patch works well. I think we're almost done.

Two things remaining that I see:

1. We need feedback on whether the approach of dynamically building the vertical tabs is desired.
2. There is still a css float issue caused by the commerce_discount_usage.css file. (see screenshot). Since the only css in that file is causing the float problem, I believe the whole file can be removed.

Here is my first attempt at the new (non-automated) approach. Having to move & unset fields, then move them back during validation seems a bit sloppy, but I didn't see another obvious approach without significantly altering the original form.

@smccabe - I've removed the commerce_discount_usage.css file as @rszrama recommended, but I'm still having issues with sass compilation replacing the whole line, so I didn't make changes to the remaining css. It must have something to do with my setup. I suspect in the end, you will have to roll the final patch along with css changes.

@rszrama - I started working on commerce_coupon 2.x, but it has a much more complicated form situation, so I'd like to hear thoughts on this patch before going much further.

Any feedback is appreciated.

@rszrama - Yes, the difficulty is in putting multiple fields that already exist in the form structure into a single vertical tab. I couldn't find a way to make the fields appear within the vertical tab without actually moving them. Is there something obvious I've missed?

The javascript for vertical tab gutters shouldn't be too difficult. I can dig into it once I get the fields moved in a reasonable way.

This fixes nothing other than the nitpicks/coding standards, I've gotta catch up on what's been done. There were some errant whitespace at the end of lines and 80chars, etc stuffs.

Thanks for giving this a go, keep going @daggerhart;)

Thanks for @bojanz in IRC this patch takes a much more clean approach. It creates the tab fieldsets in hook_form_alter so that vertical tab weights are respected, but doesn't move the field to those tabs until #pre_render so that we don't have to move the fields back during validation.

I didn't touched the css this time because everything is looking good without doing so. Any feedback is appreciated.

Patch #22 looks great! Marking review to see if test bot is still upset.

#22 accomplishes the tasks and keeps the tests a passin'!

@daggerhart this looks great thanks for the clean-up. Interesting approach, and just curious if you played with the #parents

Some references if you wanted to play with it at all:
https://www.drupal.org/node/48643
https://www.drupal.org/node/279246#comment-2292322

Thanks all, I've committed this to the latest dev branch. Any further changes can be done in a follow-up if you see fit.

It's so clean and so pretty! The new config form is soooo much friendlier now, between the vertical tabs and the CSS cleanup. Nice work!