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

Add translation progress bar #40

Merged
merged 3 commits into from
Jul 13, 2021

Conversation

jennchenn
Copy link
Collaborator

@jennchenn jennchenn commented Jul 2, 2021

Notion ticket link

Add translation progress state

Implementation description

  • Added progress bar to TranslationPage in frontend
  • Counting and returning number of translated lines on storyTranslationById query (added numTranslatedLines to StoryTranslationResponseDTO)
  • Will be building implementation in frontend to reflect/update percentage progress bar in frontend after PR Create TranslationPage for translating story content #39 is merged

Steps to test

  1. Seed database with data:
$ INSERT INTO users (first_name, last_name, auth_id, role, approved_languages) VALUES ('PlanetAdmin', 'Read', <auth id>, <role>, '{"HINDI":3}');
$ INSERT INTO stories (title, description, youtube_link, level, translated_languages) VALUES ('A Book for Puchku', 'Puchku has run out of books to read. Then she discovers more books in the top shelf of the library bookcase. But Puchku is small, and the bookcase tall. How will she ever get to her beloved books?', 'https://www.youtube.com/watch?v=PKxMjO4HPvE', 3, '{}');
$ INSERT INTO story_contents (story_id, line_index, content) VALUES (<id of story you just inserted>,1,'first line'), (<id of story you just inserted>,2,'second line'), (<id of story you just inserted>,3,'third line'), (<id of story you just inserted>,4,'fourth line');
$ INSERT INTO story_translations (story_id, language, stage, translator_id) VALUES (<id of story>,'HINDI', 'TRANSLATE',1);
$ INSERT INTO story_translation_contents (story_translation_id, line_index, translation_content) VALUES (<id of story_translation>, 1, 'translation!');
$ INSERT INTO story_translation_contents (story_translation_id, line_index, translation_content) VALUES (<id of story_translation>, 2, '');
$ INSERT INTO story_translation_contents (story_translation_id, line_index, translation_content) VALUES (<id of story_translation>, 3, '');
  1. Check that query for storyTranslationById returns correct number of translated lines (i.e. in this case it should return 1):
{
  storyTranslationById(id: 1) {
    id
    numTranslatedLines
  }
}
  1. Verify the CSS for the progress bar when you navigate to http://localhost:3000/translation/{storyId}/{storyTranslationId} (currently hardcoded at 25%)
    image

What should reviewers focus on?

  • backend/python/app/services/implementations/story_service.py
  • frontend/src/components/translation/TranslationProgressBar.tsx

Checklist

  • My PR name is descriptive and in imperative tense
  • My commit messages are descriptive and in imperative tense. My commits are atomic and trivial commits are squashed or fixup'd into non-trivial commits
  • For backend changes, I have run the appropriate linters: docker exec -it planet-read_py-backend_1 /bin/bash -c "black . && isort --profile black ."
  • I have requested a review from the PL, as well as other devs who have background knowledge on this PR or who will be building on top of this PR

@jennchenn jennchenn requested a review from gaoxk July 2, 2021 03:28
@@ -0,0 +1,16 @@
.translation {
Copy link
Contributor

Choose a reason for hiding this comment

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

very nice looking progress bar 👌 did you run it by Shaahana?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

sorry completely slipped my mind just messaged i will make any updates according to her comments

@jennchenn jennchenn marked this pull request as draft July 5, 2021 00:54
@jennchenn jennchenn force-pushed the story-translation/add-translation-progress branch 2 times, most recently from 41a6c69 to a660e43 Compare July 12, 2021 00:46
@jennchenn jennchenn force-pushed the story-translation/add-translation-progress branch 2 times, most recently from ef89942 to 37e9acc Compare July 12, 2021 02:48
@jennchenn jennchenn marked this pull request as ready for review July 12, 2021 02:57
@jennchenn jennchenn requested a review from gaoxk July 12, 2021 02:57
@@ -245,3 +249,6 @@ def get_story_translations_available_for_review(self, language, level):
except Exception as error:
self.logger.error(str(error))
raise error

def _get_num_translated_lines(self, translation_contents):
return sum(1 for _ in translation_contents if _.translation_content)
Copy link
Collaborator

Choose a reason for hiding this comment

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

How does this handle empty strings?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

i believe Python empty strings evaluate to False so it should only sum if the translation_content is not an empty string--this seemed to be the case when i was testing but let me know if it's not working for you/if there's a more elegant way to do the check i'm happy to change it!!

Copy link
Contributor

Choose a reason for hiding this comment

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

https://stackoverflow.com/questions/12765833/counting-the-number-of-true-booleans-in-a-python-list/12766783

I'd prefer something like the [True,True,False].count(True) solution, ie len(arr) - arr.count('') might work

Copy link
Contributor

@gaoxk gaoxk left a comment

Choose a reason for hiding this comment

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

Nice! Just left some quick comments

@@ -11,3 +11,8 @@ code {
font-family: source-code-pro, Menlo, Monaco, Consolas, 'Courier New',
monospace;
}

:root {
Copy link
Contributor

Choose a reason for hiding this comment

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

nice to see variable css!

Story.description.label("description"),
Story.youtube_link.label("youtube_link"),
Story.level.label("level"),
(
Copy link
Contributor

Choose a reason for hiding this comment

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

are the outer brackets (ie () needed?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

nope removed now! i must have accidentally added it in with the rebasing/formatting

@@ -245,3 +249,6 @@ def get_story_translations_available_for_review(self, language, level):
except Exception as error:
self.logger.error(str(error))
raise error

def _get_num_translated_lines(self, translation_contents):
return sum(1 for _ in translation_contents if _.translation_content)
Copy link
Contributor

Choose a reason for hiding this comment

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

https://stackoverflow.com/questions/12765833/counting-the-number-of-true-booleans-in-a-python-list/12766783

I'd prefer something like the [True,True,False].count(True) solution, ie len(arr) - arr.count('') might work

@@ -0,0 +1,24 @@
import React from "react";
import { ProgressBar } from "react-bootstrap";
Copy link
Contributor

Choose a reason for hiding this comment

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

we haven't made any formal component library decisions so I'm happy with keeping the bootstrap. I am curious, do you have a particular reason for bootstrap or are you comfortable with it?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

oops i can definitely swap it out later once we decide on a component library! not too much thought behind it, but thought it would be easier to quickly spin up a functional progress bar
i thought i had saw bootstrap used elsewhere in the repo but i think i just imagined it now 😅
i have used it a few times before but definitely can switch it to something else!!

@@ -35,6 +36,11 @@ const App = () => {
<PrivateRoute exact path="/entity/update" component={UpdatePage} />
<PrivateRoute exact path="/entity" component={DisplayPage} />
<PrivateRoute exact path="/stories" component={HomePage} />
<PrivateRoute
Copy link
Contributor

Choose a reason for hiding this comment

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

heads up: may need rebase if jenn merges first

Copy link
Collaborator

Choose a reason for hiding this comment

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

Will rebase 👍

@jennchenn jennchenn force-pushed the story-translation/add-translation-progress branch from 37e9acc to e7db980 Compare July 13, 2021 02:39
@jennchenn jennchenn requested a review from gaoxk July 13, 2021 02:44
Copy link
Contributor

@gaoxk gaoxk left a comment

Choose a reason for hiding this comment

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

🚢 🚢

@jennchenn jennchenn merged commit 85d186a into main Jul 13, 2021
@jennchenn jennchenn deleted the story-translation/add-translation-progress branch July 13, 2021 03:34
jennchenn added a commit that referenced this pull request Jul 20, 2021
* create query to get story translation progress

* return percentage in get/update translation

* update progress bar styling
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