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

ios applink on the landing page and bottom notifciation #1090

Merged
merged 2 commits into from
May 9, 2018
Merged

ios applink on the landing page and bottom notifciation #1090

merged 2 commits into from
May 9, 2018

Conversation

owocki
Copy link
Contributor

@owocki owocki commented May 7, 2018

Description

ios applink on the landing page and bottom notifciation

screenshot: http://bits.owocki.com/292k1c3n0c1G/Screen%20Shot%202018-05-07%20at%209.15.34%20AM.png

Checklist
  • linter status: 100% pass
  • changes don't break existing behavior
  • commit message follows commit guidelines
Affected core subsystem(s)

ui

Testing
Refers/Fixes

self

@codecov
Copy link

codecov bot commented May 7, 2018 •
edited
Loading

Codecov Report

Merging #1090 into master will decrease coverage by 0.09%.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff            @@
##           master   #1090     +/-   ##
========================================
- Coverage      32%   31.9%   -0.1%     
========================================
  Files         105     105             
  Lines        7290    7685    +395     
  Branches      950    1094    +144     
========================================
+ Hits         2333    2452    +119     
- Misses       4847    5113    +266     
- Partials      110     120     +10
Impacted Files Coverage Δ
app/marketing/views.py 14.88% <0%> (-2.38%) ⬇️
app/app/settings.py 81.7% <0%> (-1.08%) ⬇️
app/dashboard/utils.py 28.71% <0%> (+0.71%) ⬆️
app/dashboard/views.py 16.64% <0%> (+0.93%) ⬆️
app/app/utils.py 22.87% <0%> (+1.05%) ⬆️
...eting/management/commands/expiration_start_work.py 82.65% <0%> (+1.22%) ⬆️
app/dashboard/helpers.py 30.72% <0%> (+1.95%) ⬆️
...eting/management/commands/assemble_leaderboards.py 53.6% <0%> (+2.39%) ⬆️

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 ae216c8...0e6ed1d. Read the comment docs.

thelostone-mc
thelostone-mc reviewed May 7, 2018
View reviewed changes
app/retail/templates/index.html Outdated
<div class="col d-flex flex-column align-items-center">
<div class="explain-img-wrapper mb-3 d-flex justify-content-center align-items-center" style="min-height: 200px;">
<a href="{% url "ios" %}">
<img style="min-height: 200px;" src="{% static "v2/images/ios/iphone_crop.png" %}" >
Copy link
Member

Choose a reason for hiding this comment

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

replace it with max-height: 1rem ? cause we are shifting to rem + that way this doesn't seem to big on mobile devices
we should move this to css file too!

thelostone-mc
thelostone-mc reviewed May 7, 2018
View reviewed changes
app/retail/templates/index.html Outdated
</div>
</div>
<div class="col d-flex flex-column align-items-center">
<div class="justify-content-left align-items-left pt-3" style="text-align: left;">
Copy link
Member

Choose a reason for hiding this comment

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

align-items-left and text-align: left; ? Do we need both

thelostone-mc
thelostone-mc reviewed May 7, 2018
View reviewed changes
app/retail/templates/index.html Outdated
<div class="row pb-5">
<div class="col d-flex flex-column align-items-center">
</div>
</div>
Copy link
Member

Choose a reason for hiding this comment

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

This seems redundant , could we remove this ?

thelostone-mc
thelostone-mc reviewed May 7, 2018
View reviewed changes
app/retail/templates/index.html Outdated
@@ -417,6 +417,35 @@ <h4 class="">{% trans "Press" %}</h4>
</div>
</div>
</div>
<div class="container-fluid pt-0" style="background-color: #eee;">
Copy link
Member

Choose a reason for hiding this comment

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

the background-color seems to be overridden here

@thelostone-mc
Copy link
Member

@owocki @mbeacom left a few comments! If it seems like a log -> we could get this merged and i'll throw in an update ^_^

@thelostone-mc
Copy link
Member

@PixelantDesign should we include this in the redesign ?

@thelostone-mc
Copy link
Member

thelostone-mc commented May 8, 2018 •
edited
Loading

Since this page is due for revamp -> I could hardcode the css and make it looks like this
Your call @owocki / @mbeacom ^_^

I removed the bottom padding to make it look it's peeking above the footer(always wanted to do that :P )

Either a)

screen shot 2018-05-08 at 4 44 19 pm

b) shorter width

screen shot 2018-05-08 at 4 55 15 pm

@owocki
Copy link
Contributor Author

owocki commented May 8, 2018

@thelostone-mc sure -- pls just update my PR !

@thelostone-mc
Copy link
Member

@mbeacom UI skill level +1

x

@thelostone-mc thelostone-mc removed their request for review May 8, 2018 16:49
mbeacom
mbeacom approved these changes May 9, 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.

lgtm :shipit:

@mbeacom mbeacom merged commit c933908 into master May 9, 2018
@mbeacom mbeacom deleted the kevin/ios branch May 9, 2018 19:53
matthewlilley pushed a commit to matthewlilley/web that referenced this pull request May 11, 2018
@owocki @matthewlilley
ios applink on the landing page and bottom notifciation (gitcoinco#1090)
5ea1c35 
* ios app on the landing page and bottom notifciation

* css: updated ios launch banner
@owocki
Copy link
Contributor Author

owocki commented May 15, 2018

shit... this was not supposed to go out until @PixelantDesign signed off on the iOS app. we are still devving it

@owocki
Copy link
Contributor Author

owocki commented May 15, 2018

5ebf6b4

@owocki
Copy link
Contributor Author

owocki commented May 15, 2018

#1167

@mbeacom
Copy link
Contributor

mbeacom commented May 15, 2018

👍

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

@thelostone-mc thelostone-mc thelostone-mc left review comments

@mbeacom mbeacom mbeacom approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@owocki @thelostone-mc @mbeacom