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 ten year map page #2317

Merged
merged 17 commits into from
Jul 18, 2024
Merged

feat: add ten year map page #2317

merged 17 commits into from
Jul 18, 2024

Conversation

evemartin
Copy link
Contributor

@evemartin evemartin commented Jun 26, 2024

This change is Reviewable

@SKairinos SKairinos closed this Jul 12, 2024
@SKairinos SKairinos reopened this Jul 12, 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 2 of 9 files at r1, 1 of 4 files at r2, 1 of 1 files at r3, 5 of 7 files at r4, 9 of 9 files at r5, all commit messages.
Reviewable status: all files reviewed, 10 unresolved discussions (waiting on @evemartin)


portal/urls.py line 533 at r5 (raw file):

        name="remove_fake_accounts",
    ),
    url(r"^tenYears/", ten_year_map_page, name="tenYears"),

Laura and I discussed this briefly last week, could we update tenYears to celebrate? Sounds / looks better as a URL. Means you'll have to replace it everywhere though, sorry 😬


portal/static/portal/img/howe_dell_1.png line 0 at r5 (raw file):
Pls minify this image. Example website to do it: https://tinypng.com/


portal/static/portal/img/howe_dell_2.png line 0 at r5 (raw file):
Pls minify


portal/static/portal/img/howe_dell_3.png line 0 at r5 (raw file):
Pls minify


portal/static/portal/sass/partials/_images.scss line 114 at r5 (raw file):

    top: 25.1%;
  }
  

There's a lot of extra whitespaces in between the different sections, please remove them 🙂


portal/templates/portal/ten_year_map.html line 54 at r5 (raw file):

                            <!-- <div class="col-sm-6 carousel-image-wrapper">
                                <img src="{% static 'portal/img/howe_dell.png' %}" alt="Illustration of person climbing steps with flag">
                            </div> -->

Remove this please

Code quote:

                            <!-- <div class="col-sm-6 carousel-image-wrapper">
                                <img src="{% static 'portal/img/howe_dell.png' %}" alt="Illustration of person climbing steps with flag">
                            </div> -->

portal/templates/portal/ten_year_map.html line 66 at r5 (raw file):

                                </div>
                            </div>
                            <div class="col-sm-6 carousel-text">

I'm assuming you've checked with Laura but I'm finding the layout of this section a bit awkward - only because the right hand side is centered and it leaves a lot of space at the top and bottom. Could the title maybe be aligned with the top of the first picture, and the Learn More button be aligned with the bottom of the second picture, and we can have the third picture be centered instead of left aligned? Not sure.

Can always leave for now though as this page is a work in progress.


portal/templates/portal/ten_year_map.html line 85 at r5 (raw file):

                                </p>
                                <div>
                                    <a href="https://www.linkedin.com/company/code-for-life-uk/posts/?feedView=all" class="button button--primary button-right-arrow">Learn more</a>

Please update this link so it opens up in a separate tab 🙏 (we normally do this with external links).


portal/templates/portal/ten_year_map.html line 108 at r5 (raw file):

                            <div class="col-sm-6 carousel-text">
                                <h5>
                                    Krakow

For future reference maybe, but can we have the accent on the "o" character for this city?


portal/static/portal/img/world_map.png line 0 at r5 (raw file):
Please delete this image if we no longer use it

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: 9 of 17 files reviewed, 10 unresolved discussions (waiting on @faucomte97 and @lauracumming)


portal/urls.py line 533 at r5 (raw file):

Previously, faucomte97 (Florian Aucomte) wrote…

Laura and I discussed this briefly last week, could we update tenYears to celebrate? Sounds / looks better as a URL. Means you'll have to replace it everywhere though, sorry 😬

Updated!


portal/static/portal/img/howe_dell_1.png line at r5 (raw file):

Previously, faucomte97 (Florian Aucomte) wrote…

Pls minify this image. Example website to do it: https://tinypng.com/

👍


portal/static/portal/img/howe_dell_2.png line at r5 (raw file):

Previously, faucomte97 (Florian Aucomte) wrote…

Pls minify

👍


portal/static/portal/img/howe_dell_3.png line at r5 (raw file):

Previously, faucomte97 (Florian Aucomte) wrote…

Pls minify

👍


portal/static/portal/sass/partials/_images.scss line 114 at r5 (raw file):

Previously, faucomte97 (Florian Aucomte) wrote…

There's a lot of extra whitespaces in between the different sections, please remove them 🙂

Removed :)


portal/templates/portal/ten_year_map.html line 54 at r5 (raw file):

Previously, faucomte97 (Florian Aucomte) wrote…

Remove this please

Removed, my bad 😓


portal/templates/portal/ten_year_map.html line 66 at r5 (raw file):

Previously, faucomte97 (Florian Aucomte) wrote…

I'm assuming you've checked with Laura but I'm finding the layout of this section a bit awkward - only because the right hand side is centered and it leaves a lot of space at the top and bottom. Could the title maybe be aligned with the top of the first picture, and the Learn More button be aligned with the bottom of the second picture, and we can have the third picture be centered instead of left aligned? Not sure.

Can always leave for now though as this page is a work in progress.

I actually have not checked yet, I was thinking maybe Laura could have a look as part of the current review process too - @lauracumming what are your thoughts on the layout? For now I've aligned the tops of the two columns and added a little padding between them!


portal/templates/portal/ten_year_map.html line 85 at r5 (raw file):

Previously, faucomte97 (Florian Aucomte) wrote…

Please update this link so it opens up in a separate tab 🙏 (we normally do this with external links).

Added :)


portal/templates/portal/ten_year_map.html line 108 at r5 (raw file):

Previously, faucomte97 (Florian Aucomte) wrote…

For future reference maybe, but can we have the accent on the "o" character for this city?

Added!


portal/static/portal/img/world_map.png line at r5 (raw file):

Previously, faucomte97 (Florian Aucomte) wrote…

Please delete this image if we no longer use it

👍

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: 9 of 17 files reviewed, 10 unresolved discussions (waiting on @faucomte97 and @lauracumming)


portal/templates/portal/ten_year_map.html line 66 at r5 (raw file):

Previously, evemartin wrote…

I actually have not checked yet, I was thinking maybe Laura could have a look as part of the current review process too - @lauracumming what are your thoughts on the layout? For now I've aligned the tops of the two columns and added a little padding between them!

image.png

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 7 of 8 files at r6, all commit messages.
Reviewable status: 16 of 17 files reviewed, 1 unresolved discussion (waiting on @lauracumming)

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 2 of 2 files at r7, all commit messages.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @evemartin and @lauracumming)


portal/templates/portal/ten_year_map.html line 55 at r7 (raw file):

                                <div class="col-sm-6">
                                    <div class="row carousel-image-wrapper">
                                        <img src="{% static 'portal/img/howe_dell_1.png' %}" style="padding-right:20px" alt="Code for Life volunteers outside Howe Dell Primary School">

Move to CSS


portal/templates/portal/ten_year_map.html line 57 at r7 (raw file):

                                        <img src="{% static 'portal/img/howe_dell_1.png' %}" style="padding-right:20px" alt="Code for Life volunteers outside Howe Dell Primary School">
                                    </div>
                                    <div class="row carousel-image-wrapper" style="margin-top:15px;padding-right:20px">

Move to CSS


portal/templates/portal/ten_year_map.html line 62 at r7 (raw file):

                                </div>
                                <div class="col-sm-6 carousel-text">
                                    <h4 style="margin-top:0rem !important;">

Move to CSS


portal/templates/portal/ten_year_map.html line 79 at r7 (raw file):

                                        selected.
                                    </p>
                                    <div style="margin-top:2rem;">

Move to CS


portal/templates/portal/ten_year_map.html line 85 at r7 (raw file):

                            </div>
                            <div class="row">
                                <div class="row carousel-image-wrapper" style="margin-top:15px;padding-left:15px; padding-right:15px">

Remove row class, move top margin to CSS

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: 15 of 17 files reviewed, 5 unresolved discussions (waiting on @faucomte97)


portal/templates/portal/ten_year_map.html line 55 at r7 (raw file):

Previously, faucomte97 (Florian Aucomte) wrote…

Move to CSS

Moved!


portal/templates/portal/ten_year_map.html line 57 at r7 (raw file):

Previously, faucomte97 (Florian Aucomte) wrote…

Move to CSS

Moved!


portal/templates/portal/ten_year_map.html line 62 at r7 (raw file):

Previously, faucomte97 (Florian Aucomte) wrote…

Move to CSS

Moved!


portal/templates/portal/ten_year_map.html line 79 at r7 (raw file):

Previously, faucomte97 (Florian Aucomte) wrote…

Move to CS

Moved!


portal/templates/portal/ten_year_map.html line 85 at r7 (raw file):

Previously, faucomte97 (Florian Aucomte) wrote…

Remove row class, move top margin to CSS

Moved!

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 all commit messages.
Reviewable status: 15 of 17 files reviewed, 2 unresolved discussions (waiting on @evemartin)


portal/static/portal/sass/partials/_images.scss line 96 at r8 (raw file):

.ten-year-carousel-image--column {
  padding-right: 20px;
}

Remove ten-year from these class names - in essence we're defining styles for a carousel, regardless of what the content for it is.

Code quote:

.ten-year-button {
  margin-top: 2rem;
}

.ten-year-carousel-image {
  margin-top: 15px;
}

.ten-year-carousel-image--column {
  padding-right: 20px;
}

portal/templates/portal/ten_year_map.html line 62 at r7 (raw file):

Previously, evemartin wrote…

Moved!

Unfortunately the heading is no longer top aligned with the image 😕

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 2 of 2 files at r8.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @evemartin)

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: 14 of 17 files reviewed, 2 unresolved discussions (waiting on @faucomte97)


portal/static/portal/sass/partials/_images.scss line 96 at r8 (raw file):

Previously, faucomte97 (Florian Aucomte) wrote…

Remove ten-year from these class names - in essence we're defining styles for a carousel, regardless of what the content for it is.

Good point, fixed!


portal/templates/portal/ten_year_map.html line 62 at r7 (raw file):

Previously, faucomte97 (Florian Aucomte) wrote…

Unfortunately the heading is no longer top aligned with the image 😕

Ahh I thought I could do it by getting rid of the styling since removing the rows helped with the padding/margins, but it looks like not - styling should be added back now in classes!

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 3 of 3 files at r9, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @evemartin)

@evemartin evemartin merged commit 9c7c69d into master Jul 18, 2024
7 checks passed
@evemartin evemartin deleted the add-ten-year-map-page branch July 18, 2024 11:00
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