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

105 UI #268

Merged
merged 4 commits into from
Jun 17, 2021
Merged

105 UI #268

merged 4 commits into from
Jun 17, 2021

Conversation

allishultes
Copy link
Contributor

@allishultes allishultes commented May 27, 2021

Is your Pull Request request related to another issue in this repository ?

See #105 in Storybook

Describe what the PR does

State whether the PR is ready for review or whether it needs extra work

Ready

Additional context

Please review with the related Storybook PR to see all of the fixes for the relevant issue.

We don't have options to customise the alerts / prompts, and so I have not been able to implement Ngan-Thi's designs exactly for these. We can only customise strings for the prompt message when users try to click links during uploads, and we only have custom string support on IE for the alert when the user tries to close the window.

To test:

  • Go to the Workspace view
  • Upload a media file
  • Try to close the tab when the file is uploading (an alert should appear)
  • Try to click on:
    • A completed transcript link (an alert should appear)
    • A programme script link (an alert should appear)
    • The "back to projects" button (an alert should appear)
    • "Transcribe media" / "Create new programme script" (an alert should NOT appear)

window.onbeforeunload = (event) => {
event.preventDefault();
if (uploadTasks.size !== 0) {
event.returnValue = 'Your file has not finished uploading';
Copy link
Contributor Author

@allishultes allishultes May 27, 2021

Choose a reason for hiding this comment

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

This only results in a custom message in Safari; there aren't ways to add custom messages for Firefox and Chrome.

@allishultes allishultes marked this pull request as ready for review May 27, 2021 09:23
Copy link
Contributor

@aniablaziak aniablaziak left a comment

Choose a reason for hiding this comment

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

Thanks Alli, this worked for me in Chrome, Firefox and Safari.

Not related to this PR: I noticed that Safari is not letting me upload some file formats (.mp3, .mp4) 🤔

Copy link
Contributor

@tamsingreen tamsingreen left a comment

Choose a reason for hiding this comment

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

Code looks good - needs changes to only show the prompt on navigating away from DPE or closing the tab as upload continues when staying within the app :)

@allishultes
Copy link
Contributor Author

allishultes commented Jun 10, 2021

Code looks good - needs changes to only show the prompt on navigating away from DPE or closing the tab as upload continues when staying within the app :)

I'm actually not able to replicate this on all files, Tamsin! I've navigated within the app when uploading two test files yesterday, and neither have finished uploading to Firestore, from what I can see. Neither xAVhAuWyNLASZ0xZLB0B nor cbR57... are in the uploads folder for our admin user. Screenshots of the associated transcripts are below; these are stuck on status: uploading.

Screenshot 2021-06-10 at 09 15 17

Screenshot 2021-06-10 at 09 14 13

Screenshot 2021-06-10 at 09 13 38

On a separate note, ideally these should probably have status:fail since they will never finish...

Copy link
Contributor

@tamsingreen tamsingreen left a comment

Choose a reason for hiding this comment

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

No prompt when I navigate elsewhere in the app; prompt when I try to close the tab 💫

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