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

[student-libraries] Exporter ui fail #31502

Merged
merged 7 commits into from Oct 29, 2019
Merged

Conversation

epeach
Copy link

@epeach epeach commented Oct 26, 2019

Description

Display an error message and ask users to re-try when publish fails.
error

Testing story

Added new tests to cover when this is expected and not expected

Reviewer Checklist:

  • Tests provide adequate coverage
  • Code is well-commented
  • New features are translatable or updates will not break translations
  • Relevant documentation has been added or updated
  • User impact is well-understood and desirable
  • Pull Request is labeled appropriately
  • Follow-up work items (including potential tech debt) are tracked and linked

@epeach epeach removed the star-labs label Oct 26, 2019
@epeach epeach changed the title Exporter ui fail [student-libraries] Exporter ui fail Oct 29, 2019
@epeach epeach changed the base branch from staging to exporter-success-page October 29, 2019 20:33
@@ -128,6 +131,7 @@ class LibraryCreationDialog extends React.Component {
libraryJson,
error => {
console.warn(`Error publishing library: ${error}`);
Copy link
Contributor

Choose a reason for hiding this comment

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

Still want this console warn now that there's UI for it?

Copy link
Author

Choose a reason for hiding this comment

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

Jessie and I chatted and we decided yes, because then we can help debug if someone writes in with issues.

<p id="error-alert" style={styles.alert}>
{i18n.libraryPublishFail()}
</p>
<div>
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is this container div needed?

Copy link
Author

Choose a reason for hiding this comment

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

I liked the idea of having this state-specific behavior in a div, but it's not necessarily needed. I'm going to leave for now to avoid delaying this PR.

@epeach epeach merged commit c91d9ea into exporter-success-page Oct 29, 2019
@epeach epeach deleted the exporter-ui-fail branch October 29, 2019 20:41
@epeach epeach mentioned this pull request Oct 29, 2019
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.

None yet

2 participants