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

Added logic & UI for creating zones, updating zones and deleting zones. #15722

Merged
merged 13 commits into from
Jul 4, 2017

Conversation

DanReyLop
Copy link
Contributor

@DanReyLop DanReyLop commented Jun 30, 2017

Fixes #14268 , fixes #14271

I've followed (i.e. shamelessly copied) @coderkevin code for products. This PR includes:

  • UI tweaks (mainly in the Header action bar).
  • Creating a new shipping zone in the server.
  • Updating a shipping zone on the server.
  • Deleting a shipping zone from the server.
  • Success notice on success, error notice on error.
  • After the zone is deleted, the user is taken back to the Shipping Settings page.

This PR does NOT include:

  • Saving the shipping methods inside a shipping zone.
  • Saving the locations inside a shipping zone.
  • Auto-ordering.

I found a weird behaviour of the action-list. If you don't change anything on the zone, and click the Save button, nothing happens, because an actionList with an empty list of nextSteps is created, and that doesn't trigger the successAction. In my mind, I would expect that the successAction would trigger immediately for those cases. What do you think Kevin?

@DanReyLop DanReyLop added Store [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. labels Jun 30, 2017
@DanReyLop DanReyLop self-assigned this Jun 30, 2017
@matticbot
Copy link
Contributor

@matticbot matticbot added the [Size] XL Probably needs to be broken down into multiple smaller issues label Jun 30, 2017
@kellychoffman
Copy link
Member

kellychoffman commented Jun 30, 2017

Looks great! Just one thing:

screen shot 2017-06-30 at 1 14 19 pm

If a zone has no locations, can the button say Add locations instead of Edit?

@kellychoffman
Copy link
Member

kellychoffman commented Jun 30, 2017

Another thing: I add a zone, save it, and then stay on that page. When press the trash icon in the header, nothing happens.

Console errors:

screen shot 2017-06-30 at 1 20 54 pm

If I leave the page, and go back, it will delete just fine.

@DanReyLop
Copy link
Contributor Author

DanReyLop commented Jun 30, 2017

Console errors:

The console errors are fixed in a Marcin's PR, so just ignore them for now :)
The trash thing is definitely a bug, let me take a look.

@DanReyLop
Copy link
Contributor Author

DanReyLop commented Jun 30, 2017

The trash thing is definitely a bug, let me take a look.

Yeah, that was easy (i.e. stupid mistake). Fixed :)

If a zone has no locations, can the button say Add locations instead of Edit?

Done :)

@Automattic Automattic deleted a comment from a8ci18n Jun 30, 2017
@kellychoffman
Copy link
Member

Works now! :shipit:

Copy link
Contributor

@marcinbot marcinbot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. Just one comment and otherwise it's ready to 🚢


const ShippingZoneHeader = ( { zone, site, onSave, translate } ) => {
const ShippingZoneHeader = ( { zone, site, onSave, onDelete, translate, isBusy, canSave, isSaving } ) => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

isBusy is not passed to this component and is only used in one place. Is it still needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nope, I renamed it to isSaving and I messed that up in rebases/merges, good catch!

@@ -106,7 +107,9 @@ const ShippingZoneLocationList = ( { siteId, loaded, translate, locations, actio
<ExtendedHeader
label={ translate( 'Zone locations' ) }
description={ translate( 'Define the places that are included in this zone.' ) } >
<Button onClick={ onAddLocation } disabled={ ! loaded } >{ translate( 'Edit locations' ) }</Button>
<Button onClick={ onAddLocation } disabled={ ! loaded } >
{ isEmpty( locations ) ? translate( 'Add locations' ) : translate( 'Edit locations' ) }
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi! I've found a possible matching string that has already been translated 46 times:
translate( 'Add Location' ) ES Score: 8

Help me improve these suggestions: react with 👎 if the suggestion doesn't make any sense, or with 👍 if it's a particularly good one (even if not implemented).

* Changed the shipping-zone-locations reducer code structure so it's easier to extend

* Added saving the zone's locations when saving a zone

* Refactored the actionList code a bit.

* Update the state when the locations are finished saving.

* Automatically orders all the shipping zones on save. (#15771)
@jameskoster
Copy link
Contributor

jameskoster commented Jul 3, 2017

Everything worked as expected! :)

Couple of points;

When adding a zone, the trash icon shouldn't be in the header bar.

Shipping zone deleted correctly

"Correctly" should probably be "successfully" but tbh, I think "Shipping zone deleted" is adequate.

Seeing it live, I don't think the zone name card is working very well. When creating a zone this shouldn't be the first thing you see, it's distracting/confusing. Instead how about we put the name card beneath locations / methods as a regular input, pre-populated based on the locations they choose. I think it needs a description as well. Something like this:

zone-name

@kellychoffman
Copy link
Member

👍 Agree with @jameskoster.

@marcinbot
Copy link
Contributor

The zone name should be in a new PR. I opened #15799

@kellychoffman
Copy link
Member

Thanks, Marcin.

@DanReyLop
Copy link
Contributor Author

When adding a zone, the trash icon shouldn't be in the header bar.

Done.

"Correctly" should probably be "successfully" but tbh, I think "Shipping zone deleted" is adequate.

Done.

Also, a small UX question: What should happen when you haven't modified anything and you click the Save button?

  • a) Instant success message.
  • b) The Save button is disabled until there's actually something to be saved.
  • c) Nothing happens when you press the button.
  • d) All of the above (?)

@kellychoffman
Copy link
Member

kellychoffman commented Jul 3, 2017

Also, a small UX question: What should happen when you haven't modified anything and you click the Save button?

a) Instant success message only. We shouldn't ever disable Save buttons. This is inline with the rest of Calypso settings as well. Mainly because: people want the piece of mind that a Save button gives them even if they didn't make any change. I blame Microsoft Word :)

@DanReyLop
Copy link
Contributor Author

DanReyLop commented Jul 4, 2017

@jameskoster I've pushed the changes you suggested.
@kellychoffman I've implemented the "instant placebo success message" when you save a zone without changes.

This is ready for a final review :)

When creating a new shipping zone, the URL is `/store/:site/zone/new`. After the user hits "Save"
and the server replies with the created zone, now the URL is updated to `/store/:site/zone/27`
(or whatever ID the zone has). This is just a cosmetic change in the URL, without triggering
any route-related code or re-rendering the page or anything.
@kellychoffman
Copy link
Member

👍 Looks good.

@kellychoffman
Copy link
Member

Can we fix this: #15722 (comment)

@DanReyLop
Copy link
Contributor Author

@kellychoffman I'd already fixed that in 5843047 . Are you still seeing Edit locations in an empty zone?

@jameskoster
Copy link
Contributor

Looks good. One minor language nitpick (sorry). When adding a zone could the success message be "Shipping zone added"?

"Shipping zone saved" is fine for other occasions.

@DanReyLop
Copy link
Contributor Author

When adding a zone could the success message be "Shipping zone added"?

Done! :)

Now that Calypso is 💚 , I'm gonna merge this, if there's more stuff to do I'll open separate PRs.

@DanReyLop DanReyLop merged commit 6905109 into master Jul 4, 2017
@matticbot matticbot removed the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label Jul 4, 2017
@alisterscott alisterscott deleted the add/14268-save-zone-changes branch October 24, 2017 04:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Size] XL Probably needs to be broken down into multiple smaller issues Store
Projects
None yet
6 participants