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

[TASK-542] Display automated transcription estimate #4873

Merged
merged 11 commits into from Apr 1, 2024

Conversation

magicznyleszek
Copy link
Member

@magicznyleszek magicznyleszek commented Mar 12, 2024

Checklist

  1. If you've added code that should be tested, add tests
  2. If you've changed APIs, update (or create!) the documentation
  3. Ensure the tests pass
  4. Make sure that your code lints and that you've followed our coding style
  5. Write a title and, if necessary, a description of your work suitable for publishing in our release notes
  6. Mention any related issues in this repository (as #ISSUE) and in other repositories (as kobotoolbox/other#ISSUE)
  7. Open an issue in the docs if there are UI/UX changes

Description

After user starts automated transcription, we now display an estimate and spinner.

Notes

Changes here:

  • Removed bem.Loading and cleaned up/refactored LoadingSpinner component
  • Added LoadingSpinner to storybook
  • Added CenteredMessage component (mainly as replacement for weird functionality in LoadingSpinner that I removed)
  • Introduce getAttachmentForProcessing utility function to use it in multiple places
    • I've created a new file for it, because adding it to any other file caused circular dependency errors
  • In the StepConfigAuto for transcript we now display a spinner and time estimate
    • I've added two utility functions to make it easier: secondsToTranscriptionEstimate and getAudioDuration

Copy link

@magicznyleszek magicznyleszek marked this pull request as ready for review March 12, 2024 23:14
import React from 'react';
import styles from './bigSpinner.module.scss';

/** Displays an animated blue spinner - for white backgrounds only. */
Copy link
Contributor

Choose a reason for hiding this comment

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

Optional, but you could make this usable on non-white backgrounds using something like this for .bigSpinner:

mask-image: radial-gradient(circle, transparent $spinner-size - (4 * $spinner-line-size), black 33%);

Copy link
Member Author

Choose a reason for hiding this comment

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

Good idea :)

Comment on lines 50 to 58
if (estimateSeconds < 45) {
return t('less than a minute');
} else if (estimateSeconds >= 45 && estimateSeconds < 75) {
return t('about 1 minute');
} else if (estimateSeconds >= 75 && estimateSeconds < 150) {
return t('about 2 minutes');
} else {
return t('about 3 minutes');
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm a little confused about the math here - the jumps between the different minutes seem inconsistent, and estimateSeconds doesn't seem to represent actual seconds. Is this based solely on the formula, or did you do some real-life testing to get these values?

This should probably also handle values greater than a hard-coded limit of 3 minutes - saying 'more than 3 minutes' would be okay if the estimateSeconds value grows inconsistently as the actual time taken goes up.

Copy link
Member Author

Choose a reason for hiding this comment

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

This is coming from https://docs.google.com/spreadsheets/d/1zjOoGBuiHr8D0M9Ua6Oqe8ByKXo1kIIu/edit#gid=774040550. But I think I've read it incorrectly. Improved now :)

Comment on lines 5 to 9
export default function BigSpinner() {
return (
<span className={styles.bigSpinner}/>
);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this need to be a separate component from <LoadingSpinner/>? The only real difference I see is the style/size of the spinner element/icon, and it's missing a label (visible or just ARIA-accessible), which <LoadingSpinner/> has. It might be worth considering adding this as a size option for <LoadingSpinner/> - or you could just give it some sort of accessible label.

Copy link
Member Author

Choose a reason for hiding this comment

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

Didn't think about incorporating it into LoadingSpinner. Good idea. I ended up improving it in greater deal than just adding big version to it :)

@LMNTL LMNTL merged commit 2d75ef4 into beta Apr 1, 2024
4 checks passed
@LMNTL LMNTL deleted the task-542-transcription-estimation branch April 1, 2024 20:09
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.

None yet

2 participants