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

Redirect to run page on errors #611

Merged
merged 3 commits into from
Mar 6, 2020
Merged

Redirect to run page on errors #611

merged 3 commits into from
Mar 6, 2020

Conversation

ThomasThelen
Copy link
Member

Redirects the user to the Tale if there is an error launching the Tale. This behaviour was built into the create-tale-modal, but we were showing the modal before loading this behavior.

This is a fix for #597 . After navigating to the run page and back the browse, users should see their Tale

Test Steps:

  1. Checkout and deploy
  2. Launch two Tales
  3. Copy + Launch a Tale
  4. After the error message make sure you're brought to the run page
  5. Navigate to Browse
  6. See your Tale
  7. Create and launch a new Tale (with the two instances still running)
  8. After the error message note that you're brought to the run page
  9. Navigate to Browse
  10. See your Tale

Side Note: I found another duplicate modal issue. After viewing the run page and going back to browse... If you copy + launch again you'll get two error modals.

Copy link
Member

@bodom0015 bodom0015 left a comment

Choose a reason for hiding this comment

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

Works as advertised 👍 LGTM

I am also seeing the duplicate error modal without copy + run, the normal create/compose error modal also seems to exhibit this behavior, but seems innocuous enough

EDIT: I've added an ID to the error modal in those 2 templates... This should hopefully prevent or at least hide the duplicate modal issue. This is because an ID selector only grabs the first match, where a Class selector returns all matches. Note that the duplicate modals are still attached to the DOM, but are never displayed to the user if an ID selector is used.

@@ -31,7 +32,12 @@ export default Component.extend({
let handleLaunchError = (tale, err) => {
console.error('Failed to launch Tale', err);
self.set("errorMessage", err.message);
$('.ui.modal.compose-error').modal('show');
$('.ui.modal.compose-error').modal({
Copy link
Member

@bodom0015 bodom0015 Mar 3, 2020

Choose a reason for hiding this comment

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

Typo here - should have been copy-error... but instead of that, let's change it to an ID to prevent the duplicate modal from showing:

Suggested change
$('.ui.modal.compose-error').modal({
$('#copy-error-modal').modal({

This should fix the duplicate modal issue for the copy-on-launch error case

@@ -150,14 +150,12 @@ export default Component.extend({
const self = this;
self.set("creatingTale", false);
self.handleError(e);
$('.ui.modal.compose-error').modal('show');
$('.ui.modal.compose-error').modal({
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
$('.ui.modal.compose-error').modal({
$('#compose-error-modal').modal({

This should fix the duplicate modal issue for the create-tale / AiWT error cases

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants