Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Tax Rate UI Updates #6902

Merged
merged 39 commits into from Sep 5, 2018
Merged

Tax Rate UI Updates #6902

merged 39 commits into from Sep 5, 2018

Conversation

@spencerfinnell
Copy link
Member

@spencerfinnell spencerfinnell commented Aug 31, 2018

  • Surfaces the fact that tax rates cannot be deleted by showing them as deactivated.
  • Enables simplified rate saving because all of the data will exist in the page. (this needs to be implemented)
- Surfaces the fact that tax rates cannot be deleted by showing them as deactivated.
- Enables simplified rate saving because all of the data will exist in the page. (this needs to be implemented)
@spencerfinnell
Copy link
Member Author

@spencerfinnell spencerfinnell commented Sep 2, 2018

Pushed some updates. Now ensures the rows are all loaded on the page (hidden if deactivated, instead of not being rendered) so all rates can be passed through the sanitization method. This can be removed in the future when individual rates are updated via AJAX when there is an action.

I tried implementing the simplified saving but for some reason am still ending up with duplicate entries when trying to change the status of an existing rate.

@spencerfinnell
Copy link
Member Author

@spencerfinnell spencerfinnell commented Sep 2, 2018

Saving seems to be working well now.

@JJJ
Copy link
Collaborator

@JJJ JJJ commented Sep 2, 2018

This is loads better, but still needs some work.

  • I strongly dislike that it's possible to add 20 tax rates and refresh the page and lose all your work. Recommend an "Are you sure" message when the page tries to unload, and/or making add/remove AJAX and have them happen immediately instead of saving on "Save Changes"
  • Having the form at the bottom of the table means sites with many tax rates need to scroll to reveal the UI. Whether that's instinctive or not is questionable. Having this on top feels better, but also doesn't feel right yet.
  • Tables can have more than 1 tbody but only 1 thead and tfoot. Not sure how much we care about semantics anymore, but noting it here just in case.

None of these are blockers to merging IMO.

@spencerfinnell
Copy link
Member Author

@spencerfinnell spencerfinnell commented Sep 2, 2018

@JJJ I agree about the UI points for sure; but not sure what to change. Experimented with it as a separate form instead of attached to the table. Having the form at the top doesn’t fix the scrolling problem because once you add one it will be out of view of there are many. I think a future iteration that filters by countries by default would help with this problem.

Definitely think more auto AJAX updates in the next iteration would be nice as well.

(P.S pretty sure official WP JS coding standards enforces trailing commas)

@pippinsplugins
Copy link
Member

@pippinsplugins pippinsplugins commented Sep 3, 2018

I'm okay with the potential for a scrolling issue. It's always been that way and it's never been a problem yet.

There are two main things I'd like to see adjusted:

  1. Do not permit navigating away from page without confirmation if any changes have been made
  2. Apply to whole country and the state/province dropdown need to be placed next to each other instead of on top of one another.

Love it

@JJJ
Copy link
Collaborator

@JJJ JJJ commented Sep 3, 2018

1 is easily achieved.
2 is less easy, simply due to lack of horizontal real estate, and width issues with showing/hiding Chosen.

Excuse my linking to a Wikipedia page, but it seems like assuming a sales tax is countrywide makes a decent default: https://en.m.wikipedia.org/wiki/List_of_countries_by_tax_rates

Many countries have a base VAT/GST/sales tax.

With that in mind, I don’t love the region being on a new line, but it’s not terrible.

@spencerfinnell
Copy link
Member Author

@spencerfinnell spencerfinnell commented Sep 3, 2018

Added a confirmation when rows are added. Doesn't mark individual changes to rows which is again something that will be improved with auto AJAX saving in the future.

The horizontal space is the issue for the side by side and really the whole UI falls apart on small devices (tables are no good for this). I'll try to branch off this and experiment with a more standard form that allows more flexibility if I can. But otherwise I think this is good to merge back in to @cklosowski's branch.

@JJJ
Copy link
Collaborator

@JJJ JJJ commented Sep 3, 2018

The AYS message incorrectly triggers when clicking "Save Changes" directly.

Steps to duplicate:

  1. add a tax rate
  2. click "Save Changes"
  3. see the AYS
@spencerfinnell
Copy link
Member Author

@spencerfinnell spencerfinnell commented Sep 3, 2018

@JJJ Try that ^

@JJJ
Copy link
Collaborator

@JJJ JJJ commented Sep 3, 2018

@spencerfinnell now it's only working intermittently, specifically in Safari.

I tried bumping the version number to bust the asset cache, but it didn't make a difference.

Oddly it does seem to work consistently in Chrome.


Edit: I've confirmed in Safari that both makeDirty and confirmUnload are firing. Looks like eddTaxRates.ays isn't doing what it's supposed to?

@JJJ
Copy link
Collaborator

@JJJ JJJ commented Sep 4, 2018

RTL support added

screen shot 2018-09-03 at 10 21 26 pm

@JJJ
Copy link
Collaborator

@JJJ JJJ commented Sep 4, 2018

Fixed some RTL issues in release/3.0, merged them to issue/6869, and merged that here.

@sunnyratilal
Copy link
Contributor

@sunnyratilal sunnyratilal commented Sep 4, 2018

I’d suggest having a toggle to display long names. In the UK, we don’t refer to regions using the abbreviations that EDD uses. E.g. Surrey in the United Kingdom is displayed as GB-SRY which isn’t a “recognised” abbreviation.

@spencerfinnell
Copy link
Member Author

@spencerfinnell spencerfinnell commented Sep 4, 2018

@JJJ Looks cool! If you haven't modeled it after https://github.com/WordPress/gutenberg/tree/master/packages/components/src/form-toggle I would take a look at that to see how they've handled some accessibility things.

@sunnyratilal I can definitely see the advantage there. The issue is that the full name isn't stored anywhere for each rate (as far as I can tell) so it would be a matter of printing each countries state list in to the page and then mapping based on the code. Not a huge deal but something to consider. Did the original implementation use the full region name?

@mintplugins
Copy link
Contributor

@mintplugins mintplugins commented Sep 4, 2018

Would it be possible to add a column for "Orders", which would show a link to a list of orders which used that tax rate? Here's a quick/dirty mockup of what I'm thinking:

screen shot 2018-09-04 at 10 31 13 am

@spencerfinnell
Copy link
Member Author

@spencerfinnell spencerfinnell commented Sep 4, 2018

@JJJ what is the render argument doing? Events can be silent on Backbone to prevent rendering so that default functionality should be levereaged if possible.

@JJJ
Copy link
Collaborator

@JJJ JJJ commented Sep 4, 2018

I’d suggest having a toggle to display long names

I strongly prefer sticking to only having 1 toggle in this UI. More than 1 starts to make this gnarly to render correctly, juggling the various states.

We could maybe modify the label text and behavior to be a "Show Extras" instead? That would show/hide inactive rates and the full-text labels of each country/region?

take a look at that to see how they've handled some accessibility things.

@spencerfinnell hopefully we don't have many of those same issues, thanks to us continuing to use an input element, and not relying on an SVG to draw our mock control. I have no doubts my design here could be improved (re: sizes, borders, contrast, etc...) but I'm also comfortable iterating on those things as feedback comes in

Would it be possible to add a column for "Orders"

I'm all for connecting things together, so here's my thoughts on how we could do this.

We eliminated the "View Order Details" link from the Order History page, mostly because it's wordy, and also because it's out-of-place in the list-table UI how it's conventionally used. We also have some width constraints with this living in Settings, as we lose a lot of screen real-estate to the table label.

I think we could go vertical more easily, without smooshing the table cells together more, though I also like the simplicity of this UI as it exists today. We also could add another link to Actions, or we could make actions more like traditional row-actions in list-tables, where they are hidden by default, and appear on :hover. That would free up a table column, too.

@JJJ
Copy link
Collaborator

@JJJ JJJ commented Sep 5, 2018

Here's a question: does this setting still need a description, or is it obvious enough we can omit it?

screen shot 2018-09-04 at 10 44 15 pm

@pippinsplugins
Copy link
Member

@pippinsplugins pippinsplugins commented Sep 5, 2018

I prefer the description being shown.

@JJJ
Copy link
Collaborator

@JJJ JJJ commented Sep 5, 2018

This is feature complete and ready to merge.

@pippinsplugins can we get a review?

jz

@JJJ JJJ requested a review from pippinsplugins Sep 5, 2018
Copy link
Member

@pippinsplugins pippinsplugins left a comment

This is lovely!

Just one thing. The base/fallback rate's tooltip needs updated. The field label is Base Rate but the tooltip refers to it as a Fallback.

Let's use Base Rate for both.

@spencerfinnell
Copy link
Member Author

@spencerfinnell spencerfinnell commented Sep 5, 2018

Updated. This is based off @cklosowski branch originally but I think @JJJ has been keeping it up to date with 3.0 so it should be able to be merged in there directly if needed.

@JJJ JJJ merged commit aee9cb2 into issue/6869 Sep 5, 2018
@JJJ JJJ deleted the tax-ux branch Sep 17, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants