Gitcoin: Issue #174 #207

Elaniobro · 2018-01-03T02:45:54Z

Description

Coding the design for the iOS landing page as provided in #137

Adding in routing for ios
- Removing redirect to google form
Adding ios template
Adding in routing for itunes
- Stubbed out redirect for itunes
Added in .mp4 video assest

Commits that need to be added to this PR:

Adding image assets
Adding ios conditional check(s) for ios specific functionality in nav.html and
nav_internal.html
/ios specific css markup
ios template markup
href update app store button urls

Checklist

linter status: 100% pass
npm run stylelint produces a slew of indentation errors on top of valid stylelint errors. These will not be fixed as part of this PR.
changes don't break existing behavior
commit message follows commit guidelines

Affected core subsystem(s)

ui

Refers/Fixes

#137
#174

Pointed ios url to ios template

Elaniobro · 2018-01-03T02:46:48Z

Still work to do. Can we add a WIP or HOLD label to this PR for now?

thelostone-mc · 2018-01-04T04:29:14Z

@Elaniobro let me know when it's done, i'll help review ^_^ I've got some time to spare

Elaniobro · 2018-01-04T15:24:55Z

Thanks @thelostone-mc feel free to pull down and let me know your thoughts so far. The CSS has been a little bit of a battle, lots of !important that are pulled in that have to be overwritten!

Elaniobro · 2018-01-06T04:20:43Z

@thelostone-mc have you had a chance to take a gander?

thelostone-mc · 2018-01-06T07:12:13Z

@Elaniobro ah sorry got held up with some work! Checking it now

thelostone-mc · 2018-01-06T07:17:24Z

app/assets/v2/css/gitcoin.css

@@ -315,3 +315,161 @@ div.button-pink {
  /* IE 10+ */
  color: lightgray;
 }
+
+.ios { 


Worth moving this to it's own file rather than keeping it in gitcoin.css?

Yup, was thinking this as well.

Moved all the ios into its own file.

thelostone-mc · 2018-01-06T07:22:09Z

app/dashboard/templates/ios.html

+            <div class="row">
+                <div class="col-md-4" style="">
+                    <div class="text-center">
+                        <div class="flat-card">


We already have cards set up here. Consider reusing them, we could improve them as well ? Just a thought

thelostone-mc · 2018-01-06T07:28:47Z

As far as I remember there was a decision to move to Open-Sans font and we are already using it for toolbox page. @algae12 are we still doing this ?
I see an extra image present behind the video ! We need to take that out

the font in the card seems a bit too huge :/ does it look better if it's shrunk down ?
in mobile view ( font seems too big ) and the cards don't seem like cards 😓 might have to add some padding to fix that ( or re-use the cards used in toolbox ),

mobile video width seems a little too small, maybe maximize it's width or landscape and portait mode for mobile.

left a few more comments ^_^

thelostone-mc · 2018-01-06T07:29:33Z

(Also not relevant here but @owocki the navbar has different heights on different page ).
Not sure how this happened 😓

Elaniobro · 2018-01-07T18:57:53Z

@thelostone-mc

re-worked the cards to use the existing styles.
Added
- ios.css
- gitcoin_hover.gif (transparent version)
  - - need stactic version of this as well.
Modified head template to accomadate
- ios.css
- card.css
- toolbox.css
Reduced the overall width of the card for mobile
Made video 100% width on smaller viewports
Fixed footer padding

Still need the video asset as well as logo asset and href values.

thelostone-mc · 2018-01-08T18:00:15Z

If we change the font to Open Sans the text would stay within the card ( @algae12 has to confirm the font )

Other than that rest looks good 👍 ^_^

Elaniobro · 2018-01-08T18:25:23Z

@thelostone-mc odd, thought I had corrected that with min-height.

Thank you for reviewing, if the font is wrong, lets get confirmation on the font to use and I will address both at the same time.

owocki · 2018-01-08T18:38:53Z

just got back from my vacation.. and i got say "holy progress batman!". awesome!

lmk if anything needed from me on this :)

codecov · 2018-01-09T03:28:33Z

Codecov Report

❗ No coverage uploaded for pull request base (master@c61aa4d). Click here to learn what that means.
The diff coverage is 0%.

@@            Coverage Diff            @@
##             master     #207   +/-   ##
=========================================
  Coverage          ?   11.74%           
=========================================
  Files             ?       69           
  Lines             ?     3423           
  Branches          ?      370           
=========================================
  Hits              ?      402           
  Misses            ?     3021           
  Partials          ?        0

Impacted Files	Coverage Δ
app/app/urls.py	`0% <ø> (ø)`
app/app/sitemap.py	`0% <ø> (ø)`
app/retail/views.py	`0% <0%> (ø)`

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 c61aa4d...3397864. Read the comment docs.

Elaniobro · 2018-01-10T03:54:25Z

@owocki added in the ogg video support, and modified the helmet logo to coincide with the design. I think this is ready to 📦 🚀

Thanks @thelostone-mc for the peer review. 🥂

owocki · 2018-01-10T04:30:15Z

reviewing now..

owocki · 2018-01-10T04:30:32Z

my first thought upon pulling up /ios on my local... "damn, that looks _nice!"

owocki · 2018-01-10T04:31:49Z

page looks good in tablet, desktop, and mobile dimensions

owocki · 2018-01-10T04:32:11Z

some margin between these two buttons would be nice http://bits.owocki.com/3T172r242p0X/Screen%20Shot%202018-01-09%20at%209.31.59%20PM.png

owocki · 2018-01-10T04:32:59Z

on second thought... i dont really see any need for the 'mobile' button in the nav

owocki · 2018-01-10T04:34:08Z

just finished looking at the LP. reviewing code now

owocki

some very minor comments... otherwise... very happy with the state of things so ffar!

owocki · 2018-01-10T04:34:29Z

README.md

 ```
+Navigate to `http://0.0.0.0:8000/`.


LGTM... althought not necessarily related to this PR

If its ok with you, I'll leave it in. I think this PR will be ready to merge 🌐 in soon.

owocki · 2018-01-10T04:34:49Z

app/app/sitemap.py

@@ -28,6 +28,7 @@ def items(self):
            'whitepaper',
            'whitepaper_access',
            '_leaderboard',
+            'ios',


good catch :)

Danke Shoen! 🎩

owocki · 2018-01-10T04:35:22Z

app/assets/v2/css/ios.css

+}
+
+.ios .video {
+    width: 100%;


inconsistent indentation

fd67299 fixes this issue.

owocki · 2018-01-10T04:37:46Z

app/dashboard/templates/shared/nav_internal.html

@@ -18,7 +18,7 @@

        <nav class="navbar navbar-expand-md navbar-dark">
            <a class="navbar-brand pt-0 pb-0" href="/">
-                    <img id="logo" src="/static/v2/images/logo_large.png" data-hover="/static/v2/images/logo_large_hover.gif" width="150" height="50" />
+                    <img id="logo" src="{% if request.path == '/ios' %}/static/v2/images/ios/gitcoin.svg{%else%}/static/v2/images/logo_large.png{% endif %}" data-hover="{% if request.path == '/ios' %}/static/v2/images/ios/gitcoin_hover.gif{%else%}/static/v2/images/logo_large_hover.gif{% endif %}" {% if not request.path == '/ios' %}width="150"{%endif%} height="50" />


this line is hard to maintain from my perspective... id rather wrap the whole <img> block inside of the {% if request.path == '/ios' %} block

72c4d59 resolves this.

owocki · 2018-01-10T04:38:41Z

package-lock.json

@@ -31,7 +31,7 @@
    "ansi-styles": {


should this file be .gitignore'd?

Could we leave it here ?

package-lock ensures that installing modules from the same package.json never results in two different installs for different users!
The node community kinda encourages folks to not ignore it

@owocki I made comments on your comments, feel free to leave more comments :)

owocki · 2018-01-10T23:25:40Z

i think i will merge this in soon... and just comment out the URL route for now... when apple finally approves the app and we have a release date.. i can uncomment it.

any other changes needed @Elaniobro @thelostone-mc ?

Elaniobro · 2018-01-10T23:29:13Z

Hey @owocki which URL do you want commented out specifically? Are we talking Mobile in the nav?

owocki · 2018-01-10T23:39:23Z

Are we talking Mobile in the nav?

yes!

Elaniobro · 2018-01-10T23:49:40Z

@owocki

thelostone-mc · 2018-01-10T23:54:32Z

Yup LGTM !!
@Elaniobro you might wanna squash the commits :P

Elaniobro · 2018-01-10T23:55:31Z

@thelostone-mc how can I? I don't think I can hit the squash & merge button....

thelostone-mc · 2018-01-11T00:10:29Z

Ah didn't notice the merge commit 😓
you could squash the last 23 commits, leave the merge commit and whatever before that as is!!

You could always cherry-picks the commits before merge individually into a new branch but bleh too much of grunt work

Elaniobro · 2018-01-11T03:25:23Z

@owocki this has been squashed, worked with @thelostone-mc to do so. 👍

Gitcoin: Issue gitcoinco#174

Elan Trybuch added 4 commits December 30, 2017 23:52

Adding ios template

4e6ccd0

Adding iOS url to sitemap

57072fc

Removed ios google redirect

5ebacfe

Pointed ios url to ios template

Adding image assets

854908f

Elan Trybuch and others added 8 commits January 3, 2018 21:46

Adding iOS specific css

3e37054

Adding ios html template markup

e8cdfa8

Adding iOS specific checks for nav funtionality

c34f1e8

Inverting ios icons

b3c6d82

Updating nav for ios specifics

b55f991

Merge branch 'master' into iOS-landing-page

2b3cd25

Merge Conflict Resolved

fb57456

Conflict resolve

6a261f7

thelostone-mc reviewed Jan 6, 2018

View reviewed changes

Elaniobro mentioned this pull request Jan 10, 2018

Code iOS landing page #174

Closed

owocki reviewed Jan 10, 2018

View reviewed changes

build out for ios landing page on gitcoin.co

3397864

Elaniobro force-pushed the iOS-landing-page branch from 6a75ad8 to 3397864 Compare January 11, 2018 02:32

owocki merged commit f1ab02c into gitcoinco:master Jan 11, 2018

ethikz pushed a commit to ethikz/web that referenced this pull request Jan 24, 2018

Merge pull request gitcoinco#207 from Elaniobro/iOS-landing-page

8937c14

Gitcoin: Issue gitcoinco#174

Gitcoin: Issue #174 #207

Gitcoin: Issue #174 #207

Conversation

Elaniobro commented Jan 3, 2018 • edited Loading

Description

Commits that need to be added to this PR:

Checklist

Affected core subsystem(s)

Refers/Fixes

Elaniobro commented Jan 3, 2018

thelostone-mc commented Jan 4, 2018

Elaniobro commented Jan 4, 2018

Elaniobro commented Jan 6, 2018

thelostone-mc commented Jan 6, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thelostone-mc commented Jan 6, 2018 • edited Loading

thelostone-mc commented Jan 6, 2018 • edited Loading

Elaniobro commented Jan 7, 2018

thelostone-mc commented Jan 8, 2018

Elaniobro commented Jan 8, 2018

owocki commented Jan 8, 2018

codecov bot commented Jan 9, 2018 • edited Loading

Codecov Report

Elaniobro commented Jan 10, 2018

owocki commented Jan 10, 2018

owocki commented Jan 10, 2018

owocki commented Jan 10, 2018

owocki commented Jan 10, 2018

owocki commented Jan 10, 2018

owocki commented Jan 10, 2018

owocki left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

owocki commented Jan 10, 2018

Elaniobro commented Jan 10, 2018

owocki commented Jan 10, 2018

Elaniobro commented Jan 10, 2018

thelostone-mc commented Jan 10, 2018

Elaniobro commented Jan 10, 2018

thelostone-mc commented Jan 11, 2018 • edited Loading

Elaniobro commented Jan 11, 2018

Elaniobro commented Jan 3, 2018 •

edited

Loading

thelostone-mc commented Jan 6, 2018 •

edited

Loading

thelostone-mc commented Jan 6, 2018 •

edited

Loading

codecov bot commented Jan 9, 2018 •

edited

Loading

thelostone-mc commented Jan 11, 2018 •

edited

Loading