For some reason this module, and only this module, breaks its entity's status selector out into a special UI element floated to the right in the screen header. We're all used to it now, but it doesn't have to be this way...

I propose that once #2557247: Add vertical tabs to the discount configuration form lands, we should move this into its normal place - a vertical tab setting much like with nodes (and every other entity type) at the bottom of the form. We can keep it using a select list or radio buttons element as opposed to a checkbox.

My one hesitation is that the other vertical tabs are all related to conditions determining the applicability of a discount, whereas status isn't quite the same. However, it's close enough and would be in the usual spot, so maybe it's ok.

CommentFileSizeAuthor
#7 discount-status-vertical-tab.gif177.18 KBmglaman
#7 move_discount_status_to-2599944-7.patch7.08 KBmglaman
#3 move_discount_status_to-2599944-3.patch2.17 KBmglaman

Comments

rszrama created an issue. See original summary.

mglaman’s picture

mglaman
Primary language English
Location WI, USA
commented
Status: Active » Postponed

#2557247: Add vertical tabs to the discount configuration form is now RTBC, but pushing this to postponed since it relies on it being committed.

mglaman’s picture

mglaman
Primary language English
Location WI, USA
commented
Status: Postponed » Needs review
StatusFileSize
new move_discount_status_to-2599944-3.patch2.17 KB

It was committed, so here goes a patch!

mglaman’s picture

mglaman
Primary language English
Location WI, USA
commented
Status: Needs review » Needs work 
+++ b/commerce_discount.module
@@ -348,6 +359,7 @@ function commerce_discount_form_commerce_discount_form_alter(&$form, $form_state
+  $fields['additional_settings']['discount_status']['status'] = $fields['status'];

Forgot to unset later.

Also doesn't save as status property due to form structure changes.

The last submitted patch, 3: move_discount_status_to-2599944-3.patch, failed testing.

joelpittet’s picture

joelpittet
he/him
Primary language English
Location Vancouver
commented

We have another issue about maybe defaulting the status to false. The position of the status seems good to me where it is currently I wouldn't really want to hide it in the vertical select.

I'd rather shove the Sort Order down there.

mglaman’s picture

mglaman
Primary language English
Location WI, USA
commented
Issue summary: View changes
Status: Needs work » Needs review
StatusFileSize
new move_discount_status_to-2599944-7.patch7.08 KB
new discount-status-vertical-tab.gif177.18 KB

Here's updated patch that rearranges vertical tabs so it's easier to put property items in there. I had already moved Sort Order into the tab, but didn't see comment #6 until I came to upload patch. So status is still in there. Can update once patch reviewed and gif reviewed.

rszrama’s picture

rszrama commented

Sort order and status can both be moved there - I like your mock-up here Matt, though I think we could use some summaries. Ahh, I see that the other patch was committed without summaries, though, so I suppose I'll need to make a follow-up for that. (Thought I put that in the comment thread but I may have just said it in IRC.)

@joelpittet Re: the current position of the status field, it's in a very poor place for two reasons: 1) it's a non-standard location compared to any status element (or form element in general) on any other Drupal form; that gives it a big usability-- and 2) it's poor standard web form UX to float elements around without good reason; the best practice is for labels and form elements to proceed vertically down a page, so dangling out there in the top right portion of the screen is ill-advised.

The only reason I can imagine it making sense over there is to facilitate quicker enabling / disabling of statuses, but I think it's a pretty poor convention for making that sort of task simpler. Better to use an Ajaxy action link on the overview page so the form doesn't have to be loaded and submitted in the first place.

joelpittet’s picture

joelpittet
he/him
Primary language English
Location Vancouver
commented
Status: Needs review » Fixed

Actually yeah, thought it over again and even tough it's up top, the right side is usually lost and disassociated with the form even though it has a prominent position that position would actually be better for just the "Status" value, instead of the form element.

I'm committing this so @nvahalik can be unblocked to work on the vert tab summaries.

  • joelpittet committed c1902c5 on 7.x-1.x authored by mglaman 
    Issue #2599944 by mglaman, rszrama: Move discount status to a vertical...

Status: Fixed » Closed (fixed)

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