Easy-digital-downloads: Improve layout of variable prices metabox for small screens

[cklosowski]

With the addition of exposing the Price ID (#2610) as well as a 'default' price option (#2668), the Variable Pricing Grid is getting to the point where additional columns from extensions could get crowded:

I'm proposing/opening discussion on moving the `edd_download_price_table_row hook, which is used by extensions that add to variable pricing columns, to it's own <tr> that has a label of additional options or something to that effect. This would allow the core settings to be in one row of the table, and anything added by extensions to be in another.

Using the same hook should prove to provide a decent amount of backwards compat and putting it in a new <tr> won't affect extensions that only output new <td> items, as they are expected to be nested.

Thoughts?

[pippinsplugins]

I don't think it should be a separate TR, but rather a DIV that we force onto it's own line. There is quite a bit of javascript that depends on the TRs for counting the variable prices, adding new ones, etc. Adding additional TRs for the other options could cause issues there.

[cklosowski]

Ah, the only concern I have there is that the extensions using the hooks, could be depending on being within a TR, not a DIV, so you could end up with a TD within a DIV, and no TR to wrap it.

[spencerfinnell]

What might be cool is removing the table concept and moving more towards how widgets/menus work.

You have draggable items (which are known to be draggable based on the rest of WP) that can be rearranged. When collapsed they show the price name and ID and when expanded there is plenty of room to show whatever else is needed.

[pippinsplugins]

That is a really cool idea.

On Sunday, October 12, 2014, Spencer Finnell [email protected]
wrote:

What might be cool is removing the table concept and moving more towards
how widgets/menus work.

You have draggable items (which are known to be draggable based on the
rest of WP) that can be rearranged. When collapsed they show the item and
ID and when expanded there is plenty of room to show whatever else is
needed.

—
Reply to this email directly or view it on GitHub
https://github.com/easydigitaldownloads/Easy-Digital-Downloads/issues/2704#issuecomment-58812964
.

[cklosowski]

I like that a lot. We'd have to deff get the extension developers on board but I think it could really improve that UX/UI of variable products.

[pippinsplugins]

An example of how crowded this gets with the current UI, especially on smaller screens:

[evertiro]

I'm on board with migrating to the widget UI model!

[pippinsplugins]

Dependent upon #2881

[pippinsplugins]

Since #2881 has been punted to 2.4, I'm punting this as well.

[pippinsplugins]

Another example of how this behaves on small screens :(

[pippinsplugins]

I'm going to try and make some more progress here. New screenshot after some additional options have been added to Recurring Payments:

[SDavisMedia]

Note: Redesign on https://github.com/easydigitaldownloads/edd-recurring/issues/565 and Simple Shipping.

[SDavisMedia]

@pippinsplugins I've been trying to avoid "giving up" on this but I just don't think a temporary CSS job is going to work here. I was stabbing at it all weekend.

We have EDD core control of the markup all the way down to the hook where extensions insert their td elements to add to the table. I've tried a couple of different approaches to reduce the footprint of the table markup so that it wraps nothing but what the extensions already add... making backwards compatibility a non-issue.

The problem is even in doing that, it's what the extensions add that causes the problem we're trying to solve. Because they all add table headings th and table data td elements through the hook, they're all simply simply dumping those elements in the respective table rows tr for the table's heading and body. What you end up with is several side by side tds from multiple extensions and no way to group them appropriately... not structurally and not visually, since you can't actually place tds wherever you want them in an HTML table using CSS.

With Recurring, Simple Shipping, and SL activated, no amount of shrinking the text or anything is going to make it better. Unfortunately, it doesn't even get much better when one of those is deactivated. Keep in mind, I'm saying this is _after_ moving our default price option fields out of the mix since we have control over that markup: https://cl.ly/1P3v2c2a3l0E

I think we need to skip over a CSS patch and go straight for the heart of the problem. There are a few things I think need to happen:

1. EDD's core price option fields need to be separated (structurally) from fields added by extensions. Their display should never be changed because of an extension unless we have some sort of filter in place that adjusts them in place.

2. I'd love to see extensions add their settings with some sort of context, meaning there should be a clear way to tell (visually and programmatically) which extension a setting is coming from. I'm thinking extensions register a section, then adds its fields to that section only.

3. No HTML tables involved... even if it opens and closes within an extension's section. They're just not necessary.

--

I don't mind the widget idea mentioned previously. That's pretty much the same approach I'm suggesting. But bottom line, I think we need to follow the same concept as our EDD Extensions Settings sections.

[pippinsplugins]

Ok let's do it all the way. If that means updating all extensions in one go, so be it.

Here are the extensions I'm aware of that add options:

• Recurring Payments
• Software Licensing
• Simple Shipping
• Variable Price Descriptions

Have I missed any?

[SDavisMedia]

@pippinsplugins I hadn't thought of Variable Price Descriptions. The other three are all I could think of. Pretty sure that's it.

[pippinsplugins]

@SDavisMedia work your magic sir :+1:

We can start by redesigning what we believe it _should_ look like and how it should behave and then go from there for accounting for backwards compatibility.

[chriscct7]

Just a suggestion, the way WooCommerce handles this works/scales very well. The best way to handle this might be to consider having a tabbed interface, where each extension registers a tab, and the main pricing stuff from EDD core is the default opened tab. If no one is adding a tab (for example a site with just core installed) we could simply hide the tabs on the left, leaving just a simple table like we have now.

[SDavisMedia]

@chriscct7 absolutely. That's exactly what I have in mind. It'll increase the overall footprint of price options over what we have now but I think it's worth it.

[spencerfinnell]

I like this direction a lot: https://cl.ly/1P3v2c2a3l0E -- It removes the need for columns and allows an unlimited amount of additional settings for each price option.

Tabs would be nice but they also work in the opposite direction by reducing the amount of width available even more. A horizontal list of tabs won't work since that can't be scaled indefinitely either. I think the actual price option columns are the core of the issue and what causes 95% of the overwhelming UI.

[SDavisMedia]

@spencerfinnell can you explain a bit more why you feel my screenshot removes the need for columns? What configuration do you have in mind for those extension-specific settings?

[spencerfinnell]

The core fields (name, price, default, remove at the top would stay columns) but the other settings fields for that price option aren't really given any additional context by staying in columns. They could of course remain to an extent, and with Flexbox it will be easy to make... flexible, but stacking those settings when needed seems like a fine solution to me.

As long as there is hierarchy and you realize the current setting you are changing is for that single price option (and I think it's clear in your screenshot) then the layout of the additional settings doesn't really matter.

[SDavisMedia]

Alright, let's get it crackin'. Please read all the words before pointing anything out.

For testing, pull down issue/2704 branch, navigate to a product edit screen, and _clear that cache_. Clear it two times for good luck.

What to expect:

The goal here was to shape up the price options display. Previously, you could go from 0 to this in 2.2 seconds. That said, here's a list of what has changed visually:

• variable price option row design (single price has no display change at all... I have my reasons, we can discuss if you'd like)
• file upload row design (once price option rows changed, this had to change for consistency)
• bundled download row design (same as above)

Here are a few screenshots.

Variable pricing with default product type:

Variable pricing with bundle product type:

_Note: even when single price, the new design still applies to the Download Files metabox._

For the most part, the above screenshots are the default display for when the page first loads... even when there are extensions adding settings to the price options. Speaking of that...

Variable pricing with Software Licensing, Recurring Payments, Simple Shipping, Variable Pricing Descriptions, and a some custom code:

Each price option row has its own link for toggling the display of its additional settings.

As you can see, things fit nicely now and there's room to play. It's fully responsive as well. Also note that this was done with EDD core changes only. That's great for backwards compatibility but it means 1) we had to get a little hacky, if you want to call it that and 2) how we add settings moving forward should change.

Let's get into some details and issues.

How this is done:

Quick background info:

Previously, price options were built into an HTML table, which is why the display was rough when there was too much data. Because of this structure, extensions had to insert their settings into table <th>s and <td>s within the table. To complicate things even further, those <th>s and <td>s had to be written into the extensions themselves. The <tr>s they were inserted into were already in EDD core. So basically extensions used two hooks to add settings, like this and this.

Not only did some of our extensions approach this differently, but not all of them used classes or IDs on their HTML.

How we solved that:

So in order to make everything uniform for extensions already using the hooks, we had to do three things:

1. completely ditch HTML tables for all relevant output and replace it with more flexible markup
2. introduce an output buffer that will convert the HTML table elements sent by extensions to <span>s and <label>s where needed
3. use JS to identify as best we can where settings are coming from based on available class names, use those identifiers to build new, light HTML around them on the fly, and add section titles

This was done successfully for Software Licensing, Recurring Payments, and Simple Shipping. Variable Pricing Descriptions didn't have any identifiers but the output is clean enough to not cause any issues. Plus, it also worked as a tester for how the output buffer handles custom settings (that are already out in the wild) that I couldn't account for.

So, register all of that as what needed to happen in order to update EDD itself without breaking extensions that are already published. Keep in mind that we can update the extensions in the future to add settings that do not need to use the output buffer (light bulb moment as I type: we should add a brand new hook to the same area and mark the old two as deprecated).

Adding settings in the future:

As mentioned above, we can consider a new hook in the same location as the old hook that the actual settings fields used. But even if we don't, the following example adds settings using the old hook and hands the HTML responsibility over to the extension developers:

/**
 * Custom section for EDD price options
 */
function prefix_custom_price_option_section( $post_id, $key, $args ) {
    ?>
    <div class="edd-custom-price-option-section">
        <span class="edd-custom-price-option-section-title">Extension section title</span>
        <div class="edd-custom-price-option-section-content">
            Extension section content.
        </div>
    </div>
    <?php
}
add_action( 'edd_download_price_table_row', 'prefix_custom_price_option_section', 10, 3 );

The HTML classes in the above example are built into EDD core CSS and they match the markup that the JS hack uses when building the aforementioned extension sections on the fly.

_Note: thinking even more now, we definitely need to add a new hook so in the future, extensions can avoid the output buffer when adding settings... or else anyone adding anything that includes table HTML will have issues, most likely._

Known temporary issues:

Issue 1

Part of introducing the output buffer was introducing a new function (@cklosowski worked some serious magic) that "hijacks" all callback functions using the two hooks and runs them through a filter of sorts (not in WordPress terms) that brings the old <th> data and <td> data into the same container. In the old display, what looked like extension labels and corresponding settings was really output from two totally different rows in the HTML table. This function grabs the <th> output and moves it to where the <td> old data is outputting the settings. And like I mentioned previously, the output buffer adjusts those HTML tags.

In order to make this work, the hijack function runs on one of the two aforementioned hooks at all times, meaning I can't use something like a has_action() to check and see if there are any extensions adding settings. So we have to come up with a way to check to see if there are additional extension settings for a price option... that way we can:

• Hide the "Show advanced settings" link

• block the toggled container that would end up empty

• [x] We just need a working conditional.

Issue 2

Somewhere along the line, I broke some data specific to repeatable rows and I can't seem to find the issue. You will see this issue when you try to delete a repeatable row (doesn't matter if it's a price option row, file upload row, or bundled download row... all the same behavior).

For some reason, the system thinks there's one less row than what there really is. That means when you have two rows and try to delete one, the JS reacts as if you only have one row and prevents you from deleting it. To see this, just configure any repeatable row area to have three rows. Delete one successfully. You'll fail trying to delete one of the remaining two.

I used console.log() throughout here to check out the data and not only is the count one below what it should be, type is coming back as undefined as well. So I believe there's one mistake somewhere throwing everything off. Any help finding that would be awesome.

• [x] Fix repeatable row deleting.

Issue 3

Simple Shipping needs an enhancement sooner than later. I was forced to add one line of CSS to EDD core to hide a stray <label> it sends over. It also displays its settings based on its own toggle checkbox (I thought of this just now). So either EDD core is going to have to hide the new JS-created section somehow or it can be done in Simple Shipping. Unfortunately, I'm leaning towards the former for backwards compatibility.

• [x] Update, or account for, Simple Shipping

---

I think that's it. In summary:

• Test issue/2704 with any product configuration you can think of
• Ignore known issues, or help with them if you have some insight

[SDavisMedia]

Cc: @tnorthcutt ^^ I believe https://github.com/easydigitaldownloads/edd-recurring/issues/565 will require a refresh.

[tnorthcutt]

@SDavisMedia thanks for the heads up. Do you know if this is essentially finalized? I'd rather do one refresh than two 😁

[pippinsplugins]

@tnorthcutt It's close but let's not make any changes to the Recurring PR until this is finished. Also I'll be more than happy to adjust the PRQ for you. You've done more than enough :)

[SDavisMedia]

@tnorthcutt if you give us the green light, we'll take over.

[tnorthcutt]

@SDavisMedia @pippinsplugins that'd be _awesome_.

💚

[SDavisMedia]

@tnorthcutt okay thanks for the work you and @rclations have done so far! Much appreciated.

[tnorthcutt]

[pippinsplugins]

@SDavisMedia deleting rows is fixed.

[SDavisMedia]

@pippinsplugins interesting! I didn't think removing the modification to count would fix the lack of "type" data as well. But now I see exactly why it would. All good on my end. :+1: Thanks!

[pippinsplugins]

@SDavisMedia I've done some testing with other repeatable rows (like taxes) around EDD and so far it appears to work for me still, but let's do extensive testing to be sure.

[pippinsplugins]

@SDavisMedia after clicking Show advanced settings, can we set the curser focus to the first input field in advanced? That way the advanced link doesn't keep it's focus and the first field is ready to be used.

[SDavisMedia]

Great great great. Two of the three known issues are addressed now. Simple Shipping is next. Gonna have to add the Recurring update to the list too. I'm going to start working on both of those to see what it's gonna take.

[sunnyratilal]

Firstly, this is beautiful - great work here @SDavisMedia.

Initial round of testing gave me no errors and everything is working as expected. 👍

[cklosowski]

Ok, after some thought and discussion with @SDavisMedia, I've chosen to separate out all the JS that is strictly in place for backwards compatibility into it's own JS file (assets/js/admin-backwards-compatibility.js and assets/js/admin-backwards-compatibility.min.js). I've also taken the strings added into edd_vars and moved them to edd_backcompat_vars.

This was done for a couple reasons:
1) There is no need for someone who's site isn't triggering the cases this JS was written for to even load the code that's parsing the DOM manipulations. It's just adding load time and parsing time for no reason, so let's not do it, unless it's needed
2) By clearly separating these two layers out it's less easily confused as something we'll have to add long term support for since. You can see I've added a comment above the wp_register_script call that states they should not be relied on long term.

Test it out, see what you think. While it's registered with our other admin scripts, it's only ever enqueued when it's needed, much like we only show the Show Advanced Options view when it's needed.

[pippinsplugins]

I agree with this approach and it's all working perfectly for me!

[SDavisMedia]

Love it! Works perfectly. :+1:

[SDavisMedia]

I've been hovering around this for a couple of days and I can't find anymore issues. As far as I can tell, this is job is complete.

[SDavisMedia]

• [ ] add note for the back compat JS file to _not_ use the code

[cklosowski]

• [ ] Test with other repeatable rows.

[mintplugins]

I noticed something that I think is a bug. If you add 5 files and then remove files 1, 2 and 3, upon saving, the file keys are not reset to "1". It remains starting at 4.

[pippinsplugins]

That's actually correct. The keys there are the price IDs, which should
never change.

On Wed, Jun 21, 2017 at 2:00 PM, Phil Johnston [e-mail]
wrote:

I noticed something that I think is a bug. If you add 5 files and then
remove files 1, 2 and 3, upon saving, the file keys are not reset to "1".
It remains starting at 4.

[image: screen shot 2017-06-21 at 3 00 13 pm]
https://user-images.githubusercontent.com/1638128/27401755-5c2a52b4-5692-11e7-9203-5cabc7647dd5.png

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/easydigitaldownloads/easy-digital-downloads/issues/2704#issuecomment-310174209,
or mute the thread
https://github.com/notifications/unsubscribe-auth/AA_HfXuPbu2Z_L3pqQuaPiLgjANgOpL7ks5sGWhPgaJpZM4Cts6a
.

[mintplugins]

@pippinsplugins These are the files though - the price assignment is on the right - wouldn't those be the price IDs?

[pippinsplugins]

Sorry, I meant the file ID. That ID still doesn't change.

I do think we should add ID to Download file so it is Download file ID: 5. That makes easier to understand why they don't go 1, 2, 3, 4, etc, when inner items are deleted.

[spencerfinnell]

I think that might be redundant. The metabox title says "Download Files". I would suggest just a simple "ID: 2" and removing the draggable icon in favor of a cursor change on hover.

[SDavisMedia]

@spencerfinnell I see no reason why someone would hover over that area intuitively.

[pippinsplugins]

I'm fine with just ID: 5.

[spencerfinnell]

But WordPress! There aren't any other indicators on any other draggable items in the UI.

[SDavisMedia]

@spencerfinnell there are no other draggable items in WordPress built/designed like ours at all. Our styling matches WordPress because of course, but there's a unique aspect to the UI. I see no reason why we need to follow WordPress 100% and ignore our own approach.

[SDavisMedia]

I don't even see why we're removing Download file and Price option. Not sure how much more evidence we need that people need additional contextual help.

[spencerfinnell]

I was referring to the pattern of draggable metaboxes in WordPress -- not specifically EDD. An anchored header with a draggable header with the cursor being the visual indicator.

The whole header area should be draggable and the icon should be darker -- it's pretty hard to see currently. https://cloudup.com/ci7EO7qbJ3A

However I stand by that it's not needed. :D

[spencerfinnell]

I think Price ID would work as well. That's how it's referenced in the [purchase_link] shortcode.

[spencerfinnell]

But you of course know the types of support questions that get asked the most @SDavisMedia so your call obviously.

[SDavisMedia]

I definitely stand by what I said as well. It's our UI within WordPress... not WordPress' UI. All factors considered, I think my approach is the correct one... icon darkness and all. I'm down to do whatever gets the most votes, though.

[spencerfinnell]

My final vote would be:

• Make full header area draggable.
• Use Price ID and Download File ID as headers.
• Move the "Move" icon to the right of the "Remove" links.

[SDavisMedia]

I'm not putting a draggable icon next to a remove link. I'll do the rest and just remove the icon. I'd rather do that than debate about it. Stand by.

[SDavisMedia]

That should do it.

[cklosowski]

I think I'm ready to merge this into release/2.8 Thoughts @SDavisMedia @pippinsplugins @spencerfinnell

[pippinsplugins]

Let's merge!

[cklosowski]

Merged! Closing, but tagged with Needs Docs. This one is also on our deploy card in Trello so we can be reminded of some pre/post-release updates throughout our ecosystem.

[SDavisMedia]

Re-opening this because deleting repeatable rows is broken in some extensions that depended on core's approach. In other words, no back compat. Fixing.

[SDavisMedia]

That should do it. Chose two maintain two classes in order to introduce some new styles while keeping the old ones, mainly used by extensions.

[SDavisMedia]

Also see the above Content Restriction issue. ^^^ Not sure if it's related.

[SDavisMedia]

New PR above. Again, that fixes the extensions that still use older style repeatable rows.

But still need help figuring out if this is related or not: https://github.com/easydigitaldownloads/EDD-Content-Restriction/issues/77

[pippinsplugins]

5890 merged.

Looking into Content Restriction.