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.

Sign up for GitHub

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

Jump to bottom

landing page: final changes #1412

Merged
merged 7 commits into from
Jun 8, 2018
Merged

landing page: final changes #1412

merged 7 commits into from
Jun 8, 2018

Conversation

thelostone-mc
Copy link
Member

Description
  • updated navbar
  • swap fund & login button
  • custom newsletter
Checklist
  • linter status: 100% pass
  • changes don't break existing behavior
  • commit message follows commit guidelines
Affected core subsystem(s)
  • landing page

@ghost ghost assigned thelostone-mc Jun 8, 2018
@ghost ghost added the in progress label Jun 8, 2018
stickler-ci
stickler-ci reviewed Jun 8, 2018
View reviewed changes
app/retail/views.py Outdated
@@ -54,6 +54,9 @@ def index(request):
'slides': slides,
'slideDurationInMs': 6000,
'active': 'home',
'hide_newsletter_caption': true,
Copy link

Choose a reason for hiding this comment

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

F821 undefined name 'true'

app/retail/views.py Outdated
@@ -54,6 +54,9 @@ def index(request):
'slides': slides,
'slideDurationInMs': 6000,
'active': 'home',
'hide_newsletter_caption': true,
'hide_newsletter_consent': true,
Copy link

Choose a reason for hiding this comment

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

F821 undefined name 'true'

@thelostone-mc
Copy link
Member Author

thelostone-mc commented Jun 8, 2018 •
edited
Loading

x1

@PixelantDesign

@codecov
Copy link

codecov bot commented Jun 8, 2018 •
edited
Loading

Codecov Report

Merging #1412 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #1412   +/-   ##
=======================================
  Coverage   30.26%   30.26%           
=======================================
  Files         125      125           
  Lines        9038     9038           
  Branches     1159     1159           
=======================================
  Hits         2735     2735           
  Misses       6194     6194           
  Partials      109      109
Impacted Files Coverage Δ
app/retail/views.py 39.41% <ø> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0b07b27...2d4b0c0. Read the comment docs.

newletter: incorporated changes
50bc7cb 
- flexibly remove consent and caption text
- changed position in landing page
- update relevant CSS
PixelantDesign
PixelantDesign previously approved these changes Jun 8, 2018
View reviewed changes
@mbeacom
Copy link
Contributor

mbeacom commented Jun 8, 2018

This is on staging.

mbeacom
mbeacom reviewed Jun 8, 2018
View reviewed changes
Copy link
Contributor

@mbeacom mbeacom left a comment

Choose a reason for hiding this comment

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

Other than the question about the new bools in retail/views.py, lgtm.

app/retail/views.py
@@ -54,6 +54,9 @@ def index(request):
'slides': slides,
'slideDurationInMs': 6000,
'active': 'home',
'hide_newsletter_caption': True,
Copy link
Contributor

Choose a reason for hiding this comment

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

What's the utility of this bool? Are we altering this value at any point? 🤔

Copy link
Contributor

Choose a reason for hiding this comment

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

@mbeacom I think what @thelostone-mc has done here is he is sending this boolean only to landing-page, which makes it non-hidden in all the other pages. Hence both the caption and consent are shown in every other page except the landing page.

@mbeacom
Copy link
Contributor

mbeacom commented Jun 8, 2018

screenshot 2018-06-08 15 04 39

@thelostone-mc thelostone-mc force-pushed the landing-page branch 2 times, most recently from c463088 to b351b45 Compare June 8, 2018 19:39
@ghost ghost assigned SaptakS Jun 8, 2018
SaptakS
SaptakS approved these changes Jun 8, 2018
View reviewed changes
Copy link
Contributor

@SaptakS SaptakS left a comment

Choose a reason for hiding this comment

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

LGTM

@SaptakS SaptakS merged commit 339d645 into master Jun 8, 2018
@ghost ghost removed the in progress label Jun 8, 2018
@SaptakS SaptakS deleted the landing-page branch June 8, 2018 21:16
thelostone-mc
thelostone-mc commented Jun 9, 2018
View reviewed changes
app/assets/v2/css/landing_page.css Outdated
@@ -495,10 +495,11 @@ body {
padding: 10px !important;
min-width: 18rem !important;
font-size: 16px !important;
border: 1px solid #888 !important;
}
Copy link
Member Author

Choose a reason for hiding this comment

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

@SaptakS this only changes the border for larger devices :P

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@stickler-ci stickler-ci stickler-ci left review comments

@mbeacom mbeacom mbeacom left review comments

@SaptakS SaptakS SaptakS approved these changes

@PixelantDesign PixelantDesign PixelantDesign left review comments

Assignees

@thelostone-mc thelostone-mc

@SaptakS SaptakS

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@thelostone-mc @mbeacom @PixelantDesign @SaptakS @stickler-ci