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

feat: add language dropdown #1606

Merged
merged 66 commits into from
May 9, 2024
Merged

feat: add language dropdown #1606

merged 66 commits into from
May 9, 2024

Conversation

evemartin
Copy link
Contributor

@evemartin evemartin commented Apr 25, 2024

This change is Reviewable

@evemartin evemartin changed the title Feat: add language dropdown feat: add language dropdown Apr 25, 2024
Copy link
Contributor

@faucomte97 faucomte97 left a comment

Choose a reason for hiding this comment

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

Reviewed 6 of 14 files at r1, 8 of 8 files at r3, all commit messages.
Reviewable status: all files reviewed, 10 unresolved discussions (waiting on @evemartin)


game/end_to_end_tests/game_page.py line 25 at r3 (raw file):

        assert self.on_correct_page("game_page")
        

You've got some whitespaces on this line, running Black should fix it.


game/end_to_end_tests/test_language_dropdown.py line 11 at r3 (raw file):

        self.selenium.find_element(By.ID, "language_dropdown").click()
        self.selenium.find_element(By.ID, f"language_dropdown_fr").click()

I don't think you need an f-string here


game/end_to_end_tests/test_language_dropdown.py line 14 at r3 (raw file):

        text_count = len(self.selenium.find_elements(By.XPATH, ("//*[contains(text(),'Move forwards')]")))
        assert text_count == 0

Add a new line after this line please. I think Black fixes that too


game/static/game/css/game_screen.css line 437 at r3 (raw file):

}

.tab select {

Dropdown looks good to me but double-check with Laura if you haven't already to see what she thinks.


game/static/game/js/blocklyControl.js line 56 at r3 (raw file):

};

ocargo.BlocklyControl.prototype.updateBlockLanguage = function (language_code) {

Do you need this function here? I know you call it later but I wonder if we can just have it in the other file (check my other comment).


game/static/game/js/game.js line 1397 at r3 (raw file):

function gameUpdateBlockLanguage(language_code) {
  ocargo.blocklyControl.updateBlockLanguage(language_code);

Here - maybe replace this with the core code for the function, move it up in the file and call gameUpdateBlockLanguage at the start of the setup function as well.


game/static/game/js/game.js line 1406 at r3 (raw file):

})

You have an extra white line here.


game/static/game/js/blockly/msg/js/fr.js line 439 at r3 (raw file):

Blockly.Msg["MOVE_FORWARDS_TITLE"] = "avancer";
Blockly.Msg["MOVE_FORWARDS_TOOLTIP"] = "Faire avancer la camionnette";

😭


game/templates/game/game.html line 163 at r3 (raw file):

      </label>
    </div>
    <div id="language_tab" class="tab">

I'm assuming we don't want the dropdown to show on Python-only levels, so we probably need an additional check here.


game/views/language_code_conversions.py line 1 at r3 (raw file):

language_code_dict = {

Please make a note here to only populate the languages that we have fully translations with for the custom blocks - we can comment out the ones that aren't ready.

Copy link
Contributor Author

@evemartin evemartin left a comment

Choose a reason for hiding this comment

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

Reviewable status: all files reviewed, 10 unresolved discussions (waiting on @faucomte97)


game/end_to_end_tests/game_page.py line 25 at r3 (raw file):

Previously, faucomte97 (Florian Aucomte) wrote…

You've got some whitespaces on this line, running Black should fix it.

Fixed and will push with the other changes!


game/end_to_end_tests/test_language_dropdown.py line 11 at r3 (raw file):

Previously, faucomte97 (Florian Aucomte) wrote…

I don't think you need an f-string here

Fixed and will push with the other changes!


game/end_to_end_tests/test_language_dropdown.py line 14 at r3 (raw file):

Previously, faucomte97 (Florian Aucomte) wrote…

Add a new line after this line please. I think Black fixes that too

Fixed and will push with the other changes!


game/static/game/css/game_screen.css line 437 at r3 (raw file):

Previously, faucomte97 (Florian Aucomte) wrote…

Dropdown looks good to me but double-check with Laura if you haven't already to see what she thinks.

I'll send her a screen recording to confirm!


game/static/game/js/blocklyControl.js line 56 at r3 (raw file):

Previously, faucomte97 (Florian Aucomte) wrote…

Do you need this function here? I know you call it later but I wonder if we can just have it in the other file (check my other comment).

I've moved it to the other file and it seems to work still! So I'll go with your suggested change :))


game/static/game/js/game.js line 1397 at r3 (raw file):

Previously, faucomte97 (Florian Aucomte) wrote…

Here - maybe replace this with the core code for the function, move it up in the file and call gameUpdateBlockLanguage at the start of the setup function as well.

Same as other comment!


game/static/game/js/game.js line 1406 at r3 (raw file):

Previously, faucomte97 (Florian Aucomte) wrote…

You have an extra white line here.

Fixed and will push with the other changes!


game/static/game/js/blockly/msg/js/fr.js line 439 at r3 (raw file):

Previously, faucomte97 (Florian Aucomte) wrote…

😭

I'm so sorry 😭


game/templates/game/game.html line 163 at r3 (raw file):

Previously, faucomte97 (Florian Aucomte) wrote…

I'm assuming we don't want the dropdown to show on Python-only levels, so we probably need an additional check here.

Fixed and will push with the other changes!


game/views/language_code_conversions.py line 1 at r3 (raw file):

Previously, faucomte97 (Florian Aucomte) wrote…

Please make a note here to only populate the languages that we have fully translations with for the custom blocks - we can comment out the ones that aren't ready.

Fixed and will push with the other changes!

@evemartin evemartin mentioned this pull request May 1, 2024
Copy link
Contributor

@faucomte97 faucomte97 left a comment

Choose a reason for hiding this comment

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

Reviewed 14 of 14 files at r4, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @evemartin)


game/static/game/css/backgrounds.css line 4 at r4 (raw file):

.bg--easy {
    background-color: #86ae18;

Why?

Copy link
Contributor Author

@evemartin evemartin left a comment

Choose a reason for hiding this comment

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

Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @faucomte97)


game/static/game/css/backgrounds.css line 4 at r4 (raw file):

Previously, faucomte97 (Florian Aucomte) wrote…

Why?

When it was just "background" it was covering up the dropdown menu's arrow icon - changing it to "background-color" brought the icon back (though I'm not 100% sure why this is the case)!


game/static/game/css/game_screen.css line 437 at r3 (raw file):

Previously, evemartin wrote…

I'll send her a screen recording to confirm!

I have got the okay after checking!

Copy link
Contributor

@faucomte97 faucomte97 left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewed 2 of 2 files at r5, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @evemartin)

@evemartin evemartin merged commit 71d5d88 into master May 9, 2024
5 checks passed
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.

2 participants