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

Downloading media library archive from the export page. #27581

Merged
merged 32 commits into from Oct 12, 2018

Conversation

Projects
None yet
5 participants
@southp
Contributor

southp commented Oct 3, 2018

Changes proposed in this Pull Request

This PR adds a "Export Media Library" card to the export page:
image

A user can click on the "Download" button in the card to download an archive of the media library.

In summary, it includes:

  1. Expand the siteSettings.exporter state tree with mediaExportData, including actions, selectors and reducers.
  2. Data-layer handlers for interacting with /rest/v1.1/sites/{id}/exports/media
  3. Add a <ExportMediaCard> component.

Dependencies

This PR is built on #27578.

Testing instructions

  1. Go to http://calypso.localhost:3000/settings/export/
  2. The download button should be disabled at the beginning, waiting for the endpoint response.
  3. Once receiving the response, the download button should be enabled and you can click on it to download an archive of your media library.
@matticbot

This comment has been minimized.

Show comment
Hide comment
@wpcalypsobot

This comment has been minimized.

Show comment
Hide comment
@wpcalypsobot

wpcalypsobot Oct 3, 2018

That's a great PR description, thank you so much for your effort!

Generated by 🚫 dangerJS

wpcalypsobot commented Oct 3, 2018

That's a great PR description, thank you so much for your effort!

Generated by 🚫 dangerJS

@michelleweber

This comment has been minimized.

Show comment
Hide comment
@michelleweber

michelleweber Oct 3, 2018

Member

I might add "entire" or "all the files in," but that's up to you.

What does "as an archive" mean? Will users know what that means? I wonder if we want to be more specific and mention what kind of file they'll actually end up with.

Member

michelleweber commented Oct 3, 2018

I might add "entire" or "all the files in," but that's up to you.

What does "as an archive" mean? Will users know what that means? I wonder if we want to be more specific and mention what kind of file they'll actually end up with.

@southp

This comment has been minimized.

Show comment
Hide comment
@southp

southp Oct 3, 2018

Contributor

Thanks for taking a look, @michelleweber :)

What does "as an archive" mean? Will users know what that means? I wonder if we want to be more specific and mention what kind of file they'll actually end up with.

Good question. To be specific, "an archive" here means an archive file in the tar format, which can then be extracted on users' computers. Unfortunately I don't really know how to express this in a less tech-savvy way. Could you help me rephrase it? I'd be super grateful.

Contributor

southp commented Oct 3, 2018

Thanks for taking a look, @michelleweber :)

What does "as an archive" mean? Will users know what that means? I wonder if we want to be more specific and mention what kind of file they'll actually end up with.

Good question. To be specific, "an archive" here means an archive file in the tar format, which can then be extracted on users' computers. Unfortunately I don't really know how to express this in a less tech-savvy way. Could you help me rephrase it? I'd be super grateful.

@southp southp changed the base branch from update/add-button-disabled-props-to-action-card to master Oct 4, 2018

@southp southp requested a review from Tug Oct 4, 2018

@southp

This comment has been minimized.

Show comment
Hide comment
@southp

southp Oct 8, 2018

Contributor

@michelleweber Thanks for the feedback! I've reworded it and it now looks like this:
image

Contributor

southp commented Oct 8, 2018

@michelleweber Thanks for the feedback! I've reworded it and it now looks like this:
image

@southp southp requested a review from stephanethomas Oct 11, 2018

@stephanethomas

I left several comments, but most of them are minor. Note I couldn't apply the server patch as some hunks could not be applied cleanly by the unix 'patch' utility.

@southp

This comment has been minimized.

Show comment
Hide comment
@southp

southp Oct 12, 2018

Contributor

@stephanethomas Thanks for the insightful review.

Note I couldn't apply the server patch as some hunks could not be applied cleanly by the unix 'patch' utility.

Sorry that I didn't update the test plan in advance. It is not necessary anymore since all the backend changes has been deployed. That also means the feature flag, export-media, is not necessary anymore, so I've removed it.

When you have some time, please have a revisit to see if I've addressed all your concerns the right way. Thanks :)

Contributor

southp commented Oct 12, 2018

@stephanethomas Thanks for the insightful review.

Note I couldn't apply the server patch as some hunks could not be applied cleanly by the unix 'patch' utility.

Sorry that I didn't update the test plan in advance. It is not necessary anymore since all the backend changes has been deployed. That also means the feature flag, export-media, is not necessary anymore, so I've removed it.

When you have some time, please have a revisit to see if I've addressed all your concerns the right way. Thanks :)

@stephanethomas

If I'm not mistaken, the common practices for this when using is to set clickableHeader as true so that the whole card can be clicked to expand. I've also update the sentence as Or click here to select specific content items to export to emphasize that it can be clicked.

The problem I see with this approach is that the foldable card is not used to show more information about the action that can be performed when the call-to-action (i.e. the Export All button) is clicked. On the contrary, it is used to offer a different export option to the user which also comes with its own call-to-action.

I'm not a big fan of a click here text that doesn't have link - as users may wonder if something is missing. I think we could also be more consistent among our different export sections. So right now we have the following:

01

I'd like to propose the following changes:

02

Let me know what you think of it. I will approve this pull request in the meantime as it doesn't require another review from my point of view, and anything proposed here is optional.

As an aside, it's interesting to see that exporting images is instantaneous while exporting text content - which should be much lighter - requires a job and an email to be sent to the user.

class QueryMediaExport extends Component {
static propTypes = {
mediaExportUrl: PropTypes.string.isRequired,
};

This comment has been minimized.

@stephanethomas

stephanethomas Oct 12, 2018

Member

We're still missing some props here.

@stephanethomas

stephanethomas Oct 12, 2018

Member

We're still missing some props here.

This comment has been minimized.

@southp

southp Oct 12, 2018

Contributor

Nice catch. Thanks for the eagle eyes 🦅
Updated in d7805cd.

@southp

southp Oct 12, 2018

Contributor

Nice catch. Thanks for the eagle eyes 🦅
Updated in d7805cd.

@southp

This comment has been minimized.

Show comment
Hide comment
@southp

southp Oct 12, 2018

Contributor

A summary of our discussion:
Since there is currently no easy way to wire expanding of <FoldableCard> to the wrapped child components, we will go in two steps:

  1. Remove the clickable header prop, implement the proposed new description and ship this first.
  2. Address the issue that the options of exporting specific types of content is too obscure in an individual PR.
Contributor

southp commented Oct 12, 2018

A summary of our discussion:
Since there is currently no easy way to wire expanding of <FoldableCard> to the wrapped child components, we will go in two steps:

  1. Remove the clickable header prop, implement the proposed new description and ship this first.
  2. Address the issue that the options of exporting specific types of content is too obscure in an individual PR.

@southp southp merged commit e1b6243 into master Oct 12, 2018

14 checks passed

a8c/valid-pr This check is used to prevent accidental commits to master.
Details
ci/circleci: build-jetpack-blocks Your tests passed on CircleCI!
Details
ci/circleci: build-notifications Your tests passed on CircleCI!
Details
ci/circleci: danger Your tests passed on CircleCI!
Details
ci/circleci: icfy-stats Your tests passed on CircleCI!
Details
ci/circleci: lint-and-translate Your tests passed on CircleCI!
Details
ci/circleci: setup-1 Your tests passed on CircleCI!
Details
ci/circleci: test-client-1 Your tests passed on CircleCI!
Details
ci/circleci: test-server-1 Your tests passed on CircleCI!
Details
ci/i18n Total: 8 strings (2 new). 75% already translated.
Details
ci/wp-desktop Your PR passed the wp-desktop tests on CircleCI!
Details
ci/wp-e2e-tests-canary Your PR passed the e2e tests on CircleCI!
Details
ci/wp-e2e-tests-canary-ie11 Your PR passed the e2e tests on CircleCI!
Details
ci/wp-e2e-tests-canary-safari10 Your PR passed the e2e tests on CircleCI!
Details

@southp southp deleted the add/media-export-round-2 branch Oct 12, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment