Return of the Redesign: Upgrade of the AI:MMO home page #856

faucomte97 · 2018-12-21T16:20:30Z

After successfully claiming the Student Dashboard, the Redesign strikes again by taking over the AI:MMO home page.

Did the Redesign follow the Designer's desires accordingly, or will the Reviewers stand in its way?

This change is

…styles and added create game form)

mrniket

Reviewed 1 of 27 files at r1.
Reviewable status: 1 of 27 files reviewed, 3 unresolved discussions (waiting on @faucomte97)

portal/static/portal/img/aimmo_hero.png, line 0 at r1 (raw file):
has this png been minified?

portal/views/aimmo/home.py, line 60 at r1 (raw file):

        create_game_form = forms.AddGameForm(request.POST)
        if create_game_form.is_valid():
            game = create_game_form.save(commit=False)

can we the add game logic in a helper function so that the logic flow is more readable?

portal/views/aimmo/home.py, line 64 at r1 (raw file):

            game.owner = request.user
            game.main_user = request.user
            game.save()

what happens if the game name already exists?

CelineBoudier

Reviewed 1 of 27 files at r1.
Reviewable status: 2 of 27 files reviewed, 4 unresolved discussions (waiting on @faucomte97)

portal/urls.py, line 88 at r1 (raw file):

urlpatterns = [
    url(r'^aimmo/', aimmo_home_view, name='aimmo'),

aimmo_home might work better as a name?

mrniket

Reviewed 10 of 27 files at r1.
Reviewable status: 11 of 27 files reviewed, 14 unresolved discussions (waiting on @faucomte97)

portal/urls.py, line 88 at r1 (raw file):

urlpatterns = [
    url(r'^aimmo/', aimmo_home_view, name='aimmo'),

I probably misunderstood something, but shouldn't this line be below the other aimmo one?

portal/urls.py, line 89 at r1 (raw file):

urlpatterns = [
    url(r'^aimmo/', aimmo_home_view, name='aimmo'),
    url(r'^aimmo/', include('aimmo.urls')),

this is one place where I would probably put a comment on what's happening here :P

portal/static/portal/js/join_create_game_toggle.js, line 43 at r1 (raw file):

    $('#create_new_game_button').click(function(){
        $('#join_game').addClass("hidden");

what's this for?

portal/static/portal/js/join_create_game_toggle.js, line 55 at r1 (raw file):

        if(!document.getElementById("id_name").value || document.getElementById("id_name").value == ""){
            document.getElementById("id_name").placeholder = "Give your new game a name...";
            game_name_input.css("border-color", "#ff0000");

I think we should do this with css classes

portal/static/portal/js/join_create_game_toggle.js, line 64 at r1 (raw file):

    game_name_input.click(function () {
        document.getElementById("id_name").placeholder = "";
        game_name_input.css("border-color", "#808080");

I think we should do this with css classes

portal/static/portal/js/join_create_game_toggle.js, line 66 at r1 (raw file):

        game_name_input.css("border-color", "#808080");
    })
});

new line

portal/static/portal/sass/partials/_banners.scss, line 203 at r1 (raw file):

      &:hover {
        border-bottom: 1px solid $color-text-grey;

gray 🙃

portal/static/portal/sass/partials/_buttons.scss, line 258 at r1 (raw file):

.button-group {

  button + .button,

is there any way to simplify this?

portal/static/portal/sass/partials/_buttons.scss, line 259 at r1 (raw file):

  button + .button,
  button + button,

what impact does this change have on the rest of the site?

portal/static/portal/sass/partials/_header.scss, line 155 at r1 (raw file):

    top: 0;
    width: 100%;
    z-index: 2;

this is a bit of a magic number for me as it is partly relative to the other z-indexes on our site. Could we put this in a constant/variable? https://www.sitepoint.com/dealing-constants-sass/

mrniket

Reviewed 2 of 27 files at r1.
Reviewable status: 13 of 27 files reviewed, 16 unresolved discussions (waiting on @faucomte97)

portal/static/portal/sass/partials/_utils.scss, line 216 at r1 (raw file):

      display: inline-block;
      height: 20px;
      margin: 0 6px 0 10px;

it looks like we use multiples of five in portal, was there a particular reason we have 6px here?

portal/templates/portal/aimmo_home.html, line 66 at r1 (raw file):

                <h1>Your school has been selected to trial the preview version of AI:MMO</h1>
                <p>Your testing and
                    <a target="_blank" href="https://docs.google.com/forms/d/e/1FAIpQLSdNGbf-oLanNhIqCQ-Yz7mbiTBBjX-8rpdXQUB8XIgBvwwuJg/viewform?usp=sf_link">feedback</a>

do you think it will be useful to put these links in a dict that we can reference from? It might be easier to maintain this if we have all the links in one place.

mrniket

Reviewed 2 of 27 files at r1.
Reviewable status: 15 of 27 files reviewed, 17 unresolved discussions (waiting on @faucomte97)

portal/templates/portal/play_aimmo.html, line 56 at r1 (raw file):

          {% else %}
            Following on from Rapid Router, this exciting game will build on students&rsquo; existing coding skills.
            The game will be a Massively Multiplayer Online (MMO) time travelling adventure, which will take them to the next level on their coding journey: intermediate Python.

should this be a capital I? It sounds like a title of something to me

mrniket

Reviewed 1 of 27 files at r1.
Reviewable status: 16 of 27 files reviewed, 19 unresolved discussions (waiting on @faucomte97)

portal/templates/portal/partials/aimmo_join_game_dropdown.html, line 4 at r1 (raw file):

<div class="dropdown">
    <button class="button button--regular button--secondary button--secondary--dark button--dropdown"
            id="join_game_button" data-toggle="dropdown">

if we used a new line for the id attribute, I would probably use it with the data-toggle attribute as well

portal/templates/portal/partials/aimmo_join_game_dropdown.html, line 16 at r1 (raw file):

    </ul>
</div>
{% endif %}

new line

mrniket

Reviewed 11 of 27 files at r1.
Reviewable status: all files reviewed, 20 unresolved discussions (waiting on @faucomte97)

portal/tests/pageObjects/portal/base_page.py, line 137 at r1 (raw file):

import resources_page
import aimmo_home_page

how comes this import is at the bottom?

mrniket

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

a discussion (no related file):
Can we make an analytics event for the create game button?

CelineBoudier

Reviewable status: all files reviewed, 21 unresolved discussions (waiting on @faucomte97 and @mrniket)

a discussion (no related file):

Previously, mrniket (Niket Shah) wrote…

Can we make an analytics event for the create game button?

yes please!

faucomte97

Reviewable status: all files reviewed, 21 unresolved discussions (waiting on @mrniket, @faucomte97, and @CelineBoudier)

portal/urls.py, line 88 at r1 (raw file):

Previously, mrniket (Niket Shah) wrote…

I probably misunderstood something, but shouldn't this line be below the other aimmo one?

Done.

portal/urls.py, line 88 at r1 (raw file):

Previously, CelineBoudier (Celine Boudier) wrote…

aimmo_home might work better as a name?

Done.

portal/urls.py, line 89 at r1 (raw file):

Previously, mrniket (Niket Shah) wrote…

this is one place where I would probably put a comment on what's happening here :P

Done.

portal/static/portal/img/aimmo_hero.png, line at r1 (raw file):

Previously, mrniket (Niket Shah) wrote…

has this png been minified?

Done.

portal/static/portal/js/join_create_game_toggle.js, line 43 at r1 (raw file):

Previously, mrniket (Niket Shah) wrote…

what's this for?

This is to hide the join game button when the user wants to create a new game instead.

portal/static/portal/js/join_create_game_toggle.js, line 55 at r1 (raw file):

Previously, mrniket (Niket Shah) wrote…

I think we should do this with css classes

Done.

portal/static/portal/js/join_create_game_toggle.js, line 64 at r1 (raw file):

Previously, mrniket (Niket Shah) wrote…

I think we should do this with css classes

Done.

portal/static/portal/js/join_create_game_toggle.js, line 66 at r1 (raw file):

Previously, mrniket (Niket Shah) wrote…

new line

Done.

portal/static/portal/sass/partials/_banners.scss, line 203 at r1 (raw file):

Previously, mrniket (Niket Shah) wrote…

gray 🙃

Done.

portal/static/portal/sass/partials/_utils.scss, line 216 at r1 (raw file):

Previously, mrniket (Niket Shah) wrote…

it looks like we use multiples of five in portal, was there a particular reason we have 6px here?

No particular reason at all, not sure why I put 6 instead of 5 :P I changed it to 5 and made use of the mixin.

portal/templates/portal/aimmo_home.html, line 66 at r1 (raw file):

Previously, mrniket (Niket Shah) wrote…

do you think it will be useful to put these links in a dict that we can reference from? It might be easier to maintain this if we have all the links in one place.

That's a really good idea I think. Should this be part of a separate issue do you think? This way we could move all the external links throughout the site to a separate dictionary :) If you agree I'll create an issue for it.

portal/templates/portal/play_aimmo.html, line 56 at r1 (raw file):

Previously, mrniket (Niket Shah) wrote…

should this be a capital I? It sounds like a title of something to me

I think it's OK as it is, I checked with James and he thinks it's OK too :)

portal/templates/portal/partials/aimmo_join_game_dropdown.html, line 4 at r1 (raw file):

Previously, mrniket (Niket Shah) wrote…

if we used a new line for the id attribute, I would probably use it with the data-toggle attribute as well

Done.

portal/templates/portal/partials/aimmo_join_game_dropdown.html, line 16 at r1 (raw file):

Previously, mrniket (Niket Shah) wrote…

new line

Done.

portal/tests/pageObjects/portal/base_page.py, line 137 at r1 (raw file):

Previously, mrniket (Niket Shah) wrote…

how comes this import is at the bottom?

It's actually because we can't have them at the top because of the current file organisation of the tests. base_page wants to import resources_page and resources_page wants to import base_page, and that only works if the import are at the bottom of the file. I think this became a problem because we added functionality from the menu bar (functionality that is available at any point in the website) to the base_page without creating a menu_bar file while would fix our problem. This is easily done, but I think it might be better to do this in a separate PR :) I'll create an issue if you agree.

portal/views/aimmo/home.py, line 60 at r1 (raw file):

Previously, mrniket (Niket Shah) wrote…

can we the add game logic in a helper function so that the logic flow is more readable?

Done.

faucomte97

Reviewable status: 17 of 28 files reviewed, 21 unresolved discussions (waiting on @mrniket, @CelineBoudier, and @faucomte97)

portal/views/aimmo/home.py, line 64 at r1 (raw file):

Previously, mrniket (Niket Shah) wrote…

what happens if the game name already exists?

A message now shows up explaining a game with the same name already exists :)

faucomte97

Reviewable status: 17 of 28 files reviewed, 21 unresolved discussions (waiting on @mrniket, @CelineBoudier, and @faucomte97)

a discussion (no related file):

Previously, CelineBoudier (Celine Boudier) wrote…

yes please!

Done.

faucomte97

Reviewable status: 14 of 28 files reviewed, 21 unresolved discussions (waiting on @mrniket, @CelineBoudier, and @faucomte97)

portal/static/portal/sass/partials/_buttons.scss, line 258 at r1 (raw file):

Previously, mrniket (Niket Shah) wrote…

is there any way to simplify this?

Done.

portal/static/portal/sass/partials/_buttons.scss, line 259 at r1 (raw file):

Previously, mrniket (Niket Shah) wrote…

what impact does this change have on the rest of the site?

It's now improved the margins of the button groups across the site :)

faucomte97

Reviewable status: 13 of 33 files reviewed, 20 unresolved discussions (waiting on @mrniket, @CelineBoudier, and @faucomte97)

portal/static/portal/sass/partials/_header.scss, line 155 at r1 (raw file):

Previously, mrniket (Niket Shah) wrote…

this is a bit of a magic number for me as it is partly relative to the other z-indexes on our site. Could we put this in a constant/variable? https://www.sitepoint.com/dealing-constants-sass/

Done.

CelineBoudier

Reviewable status: 13 of 33 files reviewed, 20 unresolved discussions (waiting on @mrniket, @CelineBoudier, and @faucomte97)

mrniket

Reviewed 1 of 22 files at r2.
Reviewable status: 14 of 33 files reviewed, 20 unresolved discussions (waiting on @mrniket and @faucomte97)

portal/urls.py, line 91 at r2 (raw file):

    # home page in the AIMMO project.
    # The second AIMMO URL imports all the URLs from the AIMMO project.
    url(r'^aimmo/', aimmo_home, name='aimmo'),

should we make the regex a constant in aimmo and then use that here?

portal/forms/aimmo_home.py, line 43 at r2 (raw file):

class AddGameForm(ModelForm):

As discussed, let's use the one on Aimmo :)

mrniket

Reviewed 20 of 22 files at r2.
Reviewable status: all files reviewed, 14 unresolved discussions (waiting on @faucomte97 and @mrniket)

portal/static/portal/js/join_create_game_toggle.js, line 48 at r2 (raw file):

    var back_button = $('#back_button');

    if(game_name_input.val()){

we usually put a space between the closing parenthesis and opening brackets

portal/static/portal/js/join_create_game_toggle.js, line 54 at r2 (raw file):

    }

    create_new_game_button.click(function(){

we usually put a space between the closing parenthesis and opening brackets

portal/static/portal/js/join_create_game_toggle.js, line 58 at r2 (raw file):

    });

    back_button.click(function(){

we usually put a space between the closing parenthesis and opening brackets

portal/static/portal/js/join_create_game_toggle.js, line 62 at r2 (raw file):

    });

    create_game_button.click(function(){

we usually put a space between the closing parenthesis and opening brackets

portal/static/portal/js/join_create_game_toggle.js, line 63 at r2 (raw file):

    create_game_button.click(function(){
        if(!game_name_input.val() || game_name_input.val() === ""){

we usually put a space between the closing parenthesis and opening brackets

portal/static/portal/js/join_create_game_toggle.js, line 76 at r2 (raw file):

    });

    function showCreateGameForm(){

we usually put a space between the closing parenthesis and opening brackets

portal/static/portal/js/join_create_game_toggle.js, line 81 at r2 (raw file):

    }

    function showJoinGameForm(){

we usually put a space between the closing parenthesis and opening brackets

portal/static/portal/js/join_create_game_toggle.js, line 86 at r2 (raw file):

    }

    function showInputError(error_message){

we usually put a space between the closing parenthesis and opening brackets

portal/static/portal/sass/partials/_buttons.scss, line 259 at r1 (raw file):

Previously, faucomte97 (Florian Aucomte) wrote…

It's now improved the margins of the button groups across the site :)

ah awesome! :)

portal/templates/portal/aimmo_home.html, line 66 at r1 (raw file):

Previously, faucomte97 (Florian Aucomte) wrote…

That's a really good idea I think. Should this be part of a separate issue do you think? This way we could move all the external links throughout the site to a separate dictionary :) If you agree I'll create an issue for it.

Yeah sounds good to me 👍 thanks

portal/templates/portal/base.html, line 62 at r2 (raw file):

        ga('send', 'pageview');

        function send_event(category_name, action, label_name){

space

portal/tests/pageObjects/portal/base_page.py, line 137 at r1 (raw file):

Previously, faucomte97 (Florian Aucomte) wrote…

It's actually because we can't have them at the top because of the current file organisation of the tests. base_page wants to import resources_page and resources_page wants to import base_page, and that only works if the import are at the bottom of the file. I think this became a problem because we added functionality from the menu bar (functionality that is available at any point in the website) to the base_page without creating a menu_bar file while would fix our problem. This is easily done, but I think it might be better to do this in a separate PR :) I'll create an issue if you agree.

Sure 👍 thanks!

faucomte97

Reviewable status: all files reviewed, 11 unresolved discussions (waiting on @faucomte97 and @mrniket)

portal/forms/aimmo_home.py, line 43 at r2 (raw file):

Previously, mrniket (Niket Shah) wrote…

As discussed, let's use the one on Aimmo :)

Done.

portal/static/portal/js/join_create_game_toggle.js, line 48 at r2 (raw file):

Previously, mrniket (Niket Shah) wrote…

we usually put a space between the closing parenthesis and opening brackets

Done.

portal/static/portal/js/join_create_game_toggle.js, line 54 at r2 (raw file):

Previously, mrniket (Niket Shah) wrote…

we usually put a space between the closing parenthesis and opening brackets

Done.

portal/static/portal/js/join_create_game_toggle.js, line 58 at r2 (raw file):

Previously, mrniket (Niket Shah) wrote…

we usually put a space between the closing parenthesis and opening brackets

Done.

portal/static/portal/js/join_create_game_toggle.js, line 62 at r2 (raw file):

Previously, mrniket (Niket Shah) wrote…

we usually put a space between the closing parenthesis and opening brackets

Done.

portal/static/portal/js/join_create_game_toggle.js, line 63 at r2 (raw file):

Previously, mrniket (Niket Shah) wrote…

we usually put a space between the closing parenthesis and opening brackets

Done.

portal/static/portal/js/join_create_game_toggle.js, line 76 at r2 (raw file):

Previously, mrniket (Niket Shah) wrote…

we usually put a space between the closing parenthesis and opening brackets

Done.

portal/static/portal/js/join_create_game_toggle.js, line 81 at r2 (raw file):

Previously, mrniket (Niket Shah) wrote…

we usually put a space between the closing parenthesis and opening brackets

Done.

portal/static/portal/js/join_create_game_toggle.js, line 86 at r2 (raw file):

Previously, mrniket (Niket Shah) wrote…

we usually put a space between the closing parenthesis and opening brackets

Done.

portal/templates/portal/aimmo_home.html, line 66 at r1 (raw file):

Previously, mrniket (Niket Shah) wrote…

Yeah sounds good to me 👍 thanks

Done - issue #864

portal/templates/portal/base.html, line 62 at r2 (raw file):

Previously, mrniket (Niket Shah) wrote…

space

Done.

portal/tests/pageObjects/portal/base_page.py, line 137 at r1 (raw file):

Previously, mrniket (Niket Shah) wrote…

Sure 👍 thanks!

Done - issue #865

mrniket

Reviewable status: 30 of 33 files reviewed, 11 unresolved discussions (waiting on @mrniket and @faucomte97)

portal/forms/aimmo_home.py, line 43 at r2 (raw file):

Previously, faucomte97 (Florian Aucomte) wrote…

Done.

shall we delete this form then?

mrniket

Reviewed 2 of 3 files at r3.
Reviewable status: 32 of 33 files reviewed, 2 unresolved discussions (waiting on @mrniket and @faucomte97)

mrniket

Reviewed 1 of 3 files at r3.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @faucomte97)

faucomte97

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

portal/urls.py, line 91 at r2 (raw file):

Previously, mrniket (Niket Shah) wrote…

should we make the regex a constant in aimmo and then use that here?

Yes. We will be able to import it in the portal project as soon as the latest changes of AI:MMO have been pushed to production.

portal/forms/aimmo_home.py, line 43 at r2 (raw file):

Previously, mrniket (Niket Shah) wrote…

shall we delete this form then?

We shall. We'll only be able to remove once AI:MMO is pushed to prod and portal can import the changes on AI:MMO from Pypi.

mrniket

Reviewed 9 of 9 files at r4.
Reviewable status: complete! all files reviewed, all discussions resolved

faucomte97 added 5 commits November 30, 2018 17:02

Implemented new AIMMO game page HTML, CSS & JS.

ddb3bcc

Finished redesigning AIMMO home page (added/updated PDFs, fine-tuned …

c8ff7e8

…styles and added create game form)

Make preview users always redirect to new AIMMO page.

d8a6dd2

Added tests

55844e1

Merged changes from delete base files and resolved conflicts

27ad380

faucomte97 self-assigned this Dec 21, 2018

Update SASS to cater to relevant CodeClimate issues

de318e5

mrniket suggested changes Dec 24, 2018

View reviewed changes

CelineBoudier suggested changes Dec 28, 2018

View reviewed changes

mrniket suggested changes Jan 2, 2019

View reviewed changes

CelineBoudier reviewed Jan 2, 2019

View reviewed changes

faucomte97 commented Jan 3, 2019

View reviewed changes

faucomte97 added 2 commits January 3, 2019 09:31

Updated Sass, JS and other naming instances

93ca424

Disabled creation of duplicate games with same name

cd4c489

faucomte97 commented Jan 3, 2019

View reviewed changes

Added Google Analytics event for create game button

69b1ccd

faucomte97 commented Jan 4, 2019

View reviewed changes

Simplified + selector with not(first)

b68c7c1

faucomte97 commented Jan 4, 2019

View reviewed changes

Replaced z-indices with constants

33bf1e1

faucomte97 commented Jan 8, 2019

View reviewed changes

Fixed CodeClimate issues.

cbd8566

CelineBoudier previously approved these changes Jan 9, 2019

View reviewed changes

mrniket suggested changes Jan 9, 2019

View reviewed changes

faucomte97 mentioned this pull request Jan 11, 2019

Imports can't be at the top of test files because of circular dependency #865

Closed

2 tasks

faucomte97 commented Jan 11, 2019

View reviewed changes

Added spaces between brackets

da826f4

faucomte97 dismissed CelineBoudier’s stale review via da826f4 January 11, 2019 16:12

mrniket suggested changes Jan 14, 2019

View reviewed changes

mrniket reviewed Jan 14, 2019

View reviewed changes

faucomte97 commented Jan 14, 2019

View reviewed changes

faucomte97 added 2 commits January 16, 2019 12:31

Merge master into redesign-aimmo-home

cea6812

Uses regex, AI:MMO's form and fixes CodeClimate issues

791c822

mrniket approved these changes Jan 21, 2019

View reviewed changes

faucomte97 merged commit 0cbe9bb into master Jan 21, 2019

faucomte97 deleted the redesign-aimmo-home branch January 21, 2019 10:52

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Return of the Redesign: Upgrade of the AI:MMO home page #856

Return of the Redesign: Upgrade of the AI:MMO home page #856

faucomte97 commented Dec 21, 2018 •

edited by OlafSzmidt

Loading

mrniket left a comment

CelineBoudier left a comment

mrniket left a comment

mrniket left a comment

mrniket left a comment

mrniket left a comment

mrniket left a comment

mrniket left a comment

CelineBoudier left a comment

faucomte97 left a comment

faucomte97 left a comment

faucomte97 left a comment

faucomte97 left a comment

faucomte97 left a comment

CelineBoudier left a comment

mrniket left a comment

mrniket left a comment

faucomte97 left a comment

mrniket left a comment

mrniket left a comment

mrniket left a comment

faucomte97 left a comment

mrniket left a comment

Return of the Redesign: Upgrade of the AI:MMO home page #856

Return of the Redesign: Upgrade of the AI:MMO home page #856

Conversation

faucomte97 commented Dec 21, 2018 • edited by OlafSzmidt Loading

mrniket left a comment

Choose a reason for hiding this comment

CelineBoudier left a comment

Choose a reason for hiding this comment

mrniket left a comment

Choose a reason for hiding this comment

mrniket left a comment

Choose a reason for hiding this comment

mrniket left a comment

Choose a reason for hiding this comment

mrniket left a comment

Choose a reason for hiding this comment

mrniket left a comment

Choose a reason for hiding this comment

mrniket left a comment

Choose a reason for hiding this comment

CelineBoudier left a comment

Choose a reason for hiding this comment

faucomte97 left a comment

Choose a reason for hiding this comment

faucomte97 left a comment

Choose a reason for hiding this comment

faucomte97 left a comment

Choose a reason for hiding this comment

faucomte97 left a comment

Choose a reason for hiding this comment

faucomte97 left a comment

Choose a reason for hiding this comment

CelineBoudier left a comment

Choose a reason for hiding this comment

mrniket left a comment

Choose a reason for hiding this comment

mrniket left a comment

Choose a reason for hiding this comment

faucomte97 left a comment

Choose a reason for hiding this comment

mrniket left a comment

Choose a reason for hiding this comment

mrniket left a comment

Choose a reason for hiding this comment

mrniket left a comment

Choose a reason for hiding this comment

faucomte97 left a comment

Choose a reason for hiding this comment

mrniket left a comment

Choose a reason for hiding this comment

faucomte97 commented Dec 21, 2018 •

edited by OlafSzmidt

Loading