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

Exporter: Dummy UI flow for export process #1126

Merged
merged 1 commit into from
Jan 12, 2016
Merged

Conversation

jordwest
Copy link
Contributor

@jordwest jordwest commented Dec 1, 2015

Awaiting #500, #1670
Blocks #1359

This PR implements the user interface functionality for the complete export process.

The original designs showed a popup notification upon completion, but this was decided against in favor of non-modal notices. The content of the popup has instead been moved to a CompactCard displayed below the export settings. The completion message now appears in a global notice with a button for download.

This non-modal notification will allow a user to navigate away from the export page and still receive a notice once the download file is ready.

Screenshots

screen shot 2015-12-16 at 9 16 12 pm

screen shot 2015-12-16 at 9 16 32 pm

## Export in Progress ### Basic Settings

screen shot 2015-12-16 at 9 16 25 pm

### Advanced Settings

screen shot 2015-12-16 at 9 16 37 pm

## Successful Export

screen shot 2015-12-16 at 9 18 07 pm

## Failed Export

screen shot 2015-12-16 at 9 18 35 pm

@jordwest jordwest self-assigned this Dec 1, 2015
@jordwest jordwest added this to the Export: First Release milestone Dec 1, 2015
@jordwest jordwest force-pushed the add/exporter-ui-flow branch 2 times, most recently from 9c31a2e to 28d5fdb Compare December 3, 2015 12:53
@jordwest
Copy link
Contributor Author

jordwest commented Dec 4, 2015

Would love to hear any feedback on the design @mrheston @rickybanister
cc @dllh

@jordwest jordwest added [Status] Needs Design Review Add this when you'd like to get a review / feedback from the Design team on your PR and removed [Status] In Progress labels Dec 4, 2015
@jordwest jordwest force-pushed the add/exporter-ui-flow branch from 28d5fdb to d04e3ea Compare December 4, 2015 10:09
@dllh
Copy link
Member

dllh commented Dec 4, 2015

One thing we'll need to work out is how to make the download link disappear after some period, since the export file goes away eventually. If we're just going to poll for existence of the link, then the export process could probably just fire a job to remove the data we're polling for after some period. Probably we'll also want to remove that data upon initiation of a new export too, unless this is going to be something we can use to show multiple recent export files.

I wonder if we should also make it clear that the export file will be emailed as well, or that it's ok to nav away and back to get the export file later.

@jordwest
Copy link
Contributor Author

jordwest commented Dec 4, 2015

@dllh: Good point. Currently the link will disappear if the page is reloaded, so expired links will only be an issue if the page isn't reloaded within that timeframe. Although that's a valid concern especially for the desktop app that might go weeks between refreshes. Clicking the export button again currently removes the export information.

@dllh
Copy link
Member

dllh commented Dec 4, 2015

Seems like something we can raise an issue for and not block this over. I think the same is true of my comment about setting expectations re email or navigating away.

From my perspective, the design here is great, but we should probably get a thumbs up from an actual designer before moving forward. :)

@rickybanister
Copy link

I discussed with @jordwest @dllh and got some feedback from @folletto today and we settled on using the FoldableCard component to refactor the Export interface, and putting the success and error messages in notices at the top of the page.

This screenshot shows all four states at the same time:

screen shot 2015-12-04 at 1 27 51 pm

@jordwest jordwest force-pushed the add/exporter-ui-flow branch 3 times, most recently from 54ff052 to 7ca98fc Compare December 7, 2015 20:47
@jordwest jordwest mentioned this pull request Dec 8, 2015
11 tasks
@jordwest jordwest force-pushed the add/exporter-ui-flow branch 2 times, most recently from 87ad1d8 to 4517a4c Compare December 10, 2015 10:30
@rickybanister
Copy link

There appears to be something off about the styling of the foldable card header. It seems you're using h1 and h2 for the heading and subheading.

I believe the foldable card example simply puts the main text in a div and then uses a small tag for the subtext.

Not sure if it was meant to accommodate headings.

cc @folletto

@rickybanister
Copy link

also it would be nice if we could swap the arrow for the gridicon-cog

@rickybanister
Copy link

Actually, sorry for the confusing comment regarding h1 and h2—

You can style those to be whatever you like, the component doesn't make any hard suggestions about it.

In my design I used these for the header (regardless of the wrapping tag)

font-size: 24px;
font-weight: 300;

and

font-size: 11px;

for the sub heading.

@folletto
Copy link
Contributor

You can style those to be whatever you like, the component doesn't make any hard suggestions about it.

Yeah. The component is neutral. It doesn't do any special styling in any way. :)
Each implementation has to apply the styles as needed.

also it would be nice if we could swap the arrow for the gridicon-cog

+1

@jordwest jordwest force-pushed the add/exporter-ui-flow branch from 4517a4c to 207204a Compare December 16, 2015 13:03
@jordwest jordwest added [Status] In Progress and removed [Status] Needs Design Review Add this when you'd like to get a review / feedback from the Design team on your PR labels Dec 16, 2015
@jordwest jordwest force-pushed the add/exporter-ui-flow branch from 207204a to 9ff7838 Compare December 16, 2015 14:04
@jordwest jordwest force-pushed the add/exporter-ui-flow branch from 77d7cf6 to 179c0e4 Compare December 16, 2015 18:49
@rickybanister
Copy link

I noticed two small design things here:

  1. why does the principal Export button go away when you click the cog to open the advanced settings? Since you can start many exports ad hoc, that button does not need to go away. A user might simply want to click the cog out of curiosity and then choose a full export—we shouldn't require them to click the cog a second time for that button to re-appear.
  2. The sub-heading beneath 'Feedback' that explains what is included should be moved to the right to align with the 'Feedback' heading just as the form fields align with the respective labels.

This is looking really great though!

@jordwest
Copy link
Contributor Author

@rickybanister, @folletto: I've just pushed a few changes to this PR in response to your feedback:

The screenshots in the description have been updated.

@jordwest
Copy link
Contributor Author

@rickybanister: One thing I was thinking about since we switched to radio buttons is: After selecting one of the option 'Posts', 'Pages', or 'Feedback', how would you then go back to 'All' post types?

@jordwest
Copy link
Contributor Author

Maybe the 'Export' button could be changed to say 'Export Complete Site', so that it's clear that the button won't take into account the settings below?

@rickybanister
Copy link

How about Export All

@jordwest jordwest force-pushed the add/exporter-ui-flow branch from ea559aa to f42921f Compare December 17, 2015 07:23
@jordwest jordwest added [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. and removed [Status] Needs Design Review Add this when you'd like to get a review / feedback from the Design team on your PR labels Dec 17, 2015
@jordwest jordwest force-pushed the add/exporter-ui-flow branch from f42921f to c218481 Compare December 17, 2015 15:24
@dllh
Copy link
Member

dllh commented Dec 17, 2015

I played with this a bit and it looked good to me, but it'd be good to get other eyes on the code.

@jordwest jordwest force-pushed the add/exporter-ui-flow branch from c218481 to a4cc5e6 Compare December 18, 2015 10:48
@jordwest jordwest force-pushed the add/exporter-ui-flow branch 2 times, most recently from 77a1804 to 6f9ec49 Compare January 7, 2016 12:37

export function completeExport( downloadURL ) {
notices.success(
i18n.translate( 'Your export was successful! A download link has also been sent to your email' ),
Copy link
Member

Choose a reason for hiding this comment

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

Could use a period at the end of the sentence here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

@dllh
Copy link
Member

dllh commented Jan 11, 2016

@omarjackman please review this PR and let's try to get it landed. The code looks good to me, but I've spent a lot less time writing/reading Calypso code than I feel like is necessary for a thumbs up.

// This will be replaced with polling to check when the export completes
setTimeout( () => {
dispatch( completeExport( '#', 'testing-2015-01-01.xml' ) );
//dispatch( failExport( 'The reason for failure would be displayed here' ) );
Copy link
Contributor

Choose a reason for hiding this comment

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

This would probably be a good place for a // TODO: comment instead of the commented code.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is just here for convenience - since the API calls are not connected yet, uncommenting this line and commenting out the one above it will demonstrate what happens when an export fails.
This will be removed in #1359

@omarjackman
Copy link
Contributor

@jordwest The code here looks solid to me however I haven't done much work with this new store logic to give a good review of it. Its a 👍 from me however I'd like to see someone else take a look at that portion.

@dllh
Copy link
Member

dllh commented Jan 12, 2016

Chatted with @jordwest a bit in Slack, and the store logic is going to change a fair bit in #1359. I think we should land this and request more in-depth review of the modified store logic from somebody more familiar with redux once #1359 is rebased. 🚢

@jordwest jordwest force-pushed the add/exporter-ui-flow branch from fe5deff to 425c7af Compare January 12, 2016 04:46
Squashed commits:
Exporter: Only allow one of Posts, Pages or Feedback to be selected
Exporter: Improve styling of the text under Feedback heading to match design
Exporter: Always show basic Export button, change text to clarify

The button displayed above the advanced settings is now 'Export All',
and is displayed even when the advanced settings are visible.

Exporter: Change wording to suit the switch to radios
@jordwest jordwest force-pushed the add/exporter-ui-flow branch from 425c7af to 780cb45 Compare January 12, 2016 04:48
jordwest added a commit that referenced this pull request Jan 12, 2016
Exporter: Dummy UI flow for export process
@jordwest jordwest merged commit 452196e into master Jan 12, 2016
@scruffian scruffian removed the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label Jan 12, 2016
@jordwest jordwest deleted the add/exporter-ui-flow branch January 12, 2016 05:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants