App install banner #427 #508

naveed-ahmad · 2016-11-25T05:10:11Z

This should resolved #427

Opening PR for reviews and suggestions. Sorry still learning reactjs( and hating it so far ) so bear my bad ugly for now :)

ahmedre · 2016-11-25T05:12:02Z

Deployed to: http://staging.quran.com:32941

redhae · 2016-11-25T05:43:26Z

Hi, i seem to be getting 2 banners on mobile safari

…

On 25 Nov 2016, at 1:10 PM, Naveed Ahmad ***@***.***> wrote: This should resolved #427 Opening PR for reviews and suggestions. Sorry still learning reactjs( and hating it so far ) so bear my bad ugly for now :) You can view, comment on, or merge this pull request online at: #508 Commit Summary wip on app install banner #427 File Changes A src/components/SmartBanner/index.js (193) M src/config.js (4) M src/containers/App/index.js (3) A src/styles/components/SmartBanner.scss (299) M src/styles/main.scss (1) Patch Links: https://github.com/quran/quran.com-frontend/pull/508.patch https://github.com/quran/quran.com-frontend/pull/508.diff — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub, or mute the thread.

naveed-ahmad · 2016-11-25T11:03:07Z

@redhae thanks for reporting. Did you saw two banners simultaneously or closed the banner and saw again on page refresh ?

Second case is known issue, working on that. But if first case if true, can you send screenshot or something to help me debugging ?

EDIT: Please test again. I was forcefully showing ios banner for testing purposes. Still it shouldn't two two banner :/

ahmedre · 2016-11-25T11:10:24Z

Deployed to: http://staging.quran.com:32942

thabti

Mashallah this will drive a lot of users to our apps.

LGTM!

thabti · 2016-11-26T11:14:40Z

@naveed-ahmad amazing work bro. I don't have an android to test it on. Can you include some screenshots?

Happy for this to be merged in.

naveed-ahmad · 2016-11-26T19:58:20Z

@sabeurthabti preview for different mobiles and tables:

https://drive.google.com/file/d/0B-_ddcseNUwLeHZKYU84V3lzRUk/view

ahmedre · 2016-11-26T20:08:45Z

Deployed to: http://staging.quran.com:32950

ar5had · 2016-11-26T20:36:55Z

src/styles/components/SmartBanner.scss

+  display: inline-block;
+  vertical-align: middle;
+  margin: 0 5px;
+  font-family: 'ArialRoundedMTBold', Arial;


IMO we should use font that we have used in quran.com. Font family should be either source sans pro or Montserrat or combination of both.

ar5had · 2016-11-26T20:37:04Z

src/styles/components/SmartBanner.scss

+  width: 100%;
+  height: 80px;
+  line-height: 80px;
+  font-family: 'Helvetica Neue', sans-serif;


IMO we should use font that we have used in quran.com. Font family should be either source sans pro or Montserrat or combination of both.

ar5had · 2016-11-27T17:14:21Z

@naveed-ahmad This might help you with react.

ahmedre · 2016-11-27T23:12:25Z

Deployed to: http://staging.quran.com:32952

ahmedre · 2016-11-27T23:14:08Z

Deployed to: http://staging.quran.com:32953

ahmedre · 2016-11-27T23:22:48Z

Deployed to: http://staging.quran.com:32954

naveed-ahmad · 2016-11-27T23:23:10Z

@arshdkhn1 removed the fonts, thanks for the course link.

ar5had · 2016-11-28T09:03:35Z

@naveed-ahmad

This is how banner looks in my mobile browser.

Few suggestions -

1- Quran logo is too blurry to be used.

2- I see styling is different for different browsers/os. Did you change the colors of banner recently ?

3- IMO colors should be used that are complementary to our sites' color. They are looking a bit off the color scheme that is used in site.

…m-frontend into app_install_banner

ahmedre · 2016-12-01T06:20:03Z

Deployed to: http://staging.quran.com:32971

naveed-ahmad · 2016-12-01T06:31:22Z

Android:

iOS:

ahmedre · 2016-12-01T06:36:03Z

strange, the build is failing because of the favicon for some odd reason...

 [859] ../static/images/favicon.ico 215 bytes {0} [optional] [built] [failed] [1 error]
 [860] ../static/images/titles ^\.\/.*\.svg$ 3.35 kB {0} [built]
[1577] ../static/images ^\.\/.*$ 8.2 kB {0} [built]
[1578] multi main 40 bytes {0} [built]
    + 1654 hidden modules

WARNING in ../static/images/favicon.ico
Module parse failed: /quran/static/images/favicon.ico Unexpected character '' (1:0)
You may need an appropriate loader to handle this file type.
(Source code omitted for this binary file)
 @ ../static/images ^\.\/.*$
 @ ./components/SmartBanner/index.js
 @ ./containers/App/index.js
 @ ./routes.js
 @ ./client.js
 @ multi main

ahmedre · 2016-12-01T06:37:07Z

out of curiosity, does this use whatever banner stuff Apple and Google have (i.e. will it detect if the app is installed and either not show the banner or show it with 'open' instead?)

ahmedre · 2016-12-01T06:42:50Z

Safari on iOS 10.1.1

ahmedre · 2016-12-01T06:43:51Z

not to mention for Android, the native one is a lot nicer because it also pulls in info from Google Play and looks platform native - https://developer.android.com/distribute/users/banners.html

naveed-ahmad · 2016-12-01T06:52:55Z

For android, native banner is available only for Chrome 44 and greater version:
https://developers.google.com/web/updates/2015/03/increasing-engagement-with-app-install-banners-in-chrome-for-android

And native iOS banners are available for safari 6 or greater.

ahmedre · 2016-12-01T06:55:09Z

can we do both then? native if we can, else fallback to this? sorry, i am not trying to be difficult!

naveed-ahmad · 2016-12-01T07:03:30Z

Yep working on it.

mmahalwy · 2016-12-05T08:20:56Z

@naveed-ahmad is this good to go?

naveed-ahmad · 2016-12-05T08:57:43Z

@mmahalwy No, working on showing native banner for advance browsers and fallback to custom banner for older browsers. Ill try to finish this tonight.

Conflicts: src/config.js

…m-frontend into app_install_banner

ahmedre · 2016-12-07T04:53:04Z

Deployed to: http://staging.quran.com:32981

ahmedre · 2016-12-07T05:13:21Z

Deployed to: http://staging.quran.com:32982

mmahalwy · 2016-12-07T05:19:07Z

good to go?

naveed-ahmad · 2016-12-07T05:44:31Z

@mmahalwy Gooood to go!

ahmedre · 2016-12-07T05:45:57Z

Deployed to: http://staging.quran.com:32983

ahmedre · 2016-12-07T06:13:01Z

Deployed to: http://staging.quran.com:32984

ahmedre · 2016-12-07T12:42:10Z

Deployed to: http://staging.quran.com:32985

…m-frontend into app_install_banner

ahmedre · 2016-12-07T13:09:47Z

Deployed to: http://staging.quran.com:32986

mmahalwy · 2016-12-07T21:49:15Z

good now? No more commits? :)

naveed-ahmad · 2016-12-08T07:15:01Z

@mmahalwy lol yeah good to go now.

wip on app install banner quran#427

38863f9

used fa icon for close btn

3e282bb

thabti approved these changes Nov 26, 2016

View reviewed changes

naveed-ahmad changed the title ~~wip on app install banner #427~~ App install banner #427 Nov 26, 2016

added event tracking for install and close banner

28c3a19

ar5had reviewed Nov 26, 2016

View reviewed changes

using default fonts for banner

d1ff813

naveed-ahmad force-pushed the app_install_banner branch from 6a316f1 to d1ff813 Compare November 27, 2016 23:12

Merge branch 'master' into app_install_banner

031c47a

naveed-ahmad added 3 commits December 1, 2016 11:16

fixed the banner icon and minor styling

dda4b58

Merge branch 'app_install_banner' of github.com:naveed-ahmad/quran.co…

18ce33e

…m-frontend into app_install_banner

Merge branch 'master' into app_install_banner

f65d0ac

naveed-ahmad added 4 commits December 6, 2016 23:53

added favicons, opensearch, and manifest files for chrome

16e56de

removed web from related app

07917d3

Merge branch 'meta-tags-and-branding' into app_install_banner

a1b5e3b

Conflicts: src/config.js

added service worker and preconnect meta tags

7c2adc4

naveed-ahmad mentioned this pull request Dec 7, 2016

Added favicons, opensearch, and manifest files for chrome #519

Closed

naveed-ahmad added 2 commits December 7, 2016 09:49

Merge branch 'app_install_banner' of github.com:naveed-ahmad/quran.co…

3bc7fdf

…m-frontend into app_install_banner

Merge branch 'master' into app_install_banner

453108a

don' show custom banner on chrome > 44 and safari > 6

af0e3c6

naveed-ahmad force-pushed the app_install_banner branch from 22d3296 to af0e3c6 Compare December 7, 2016 06:11

meh, when this was replaced

a12a625

naveed-ahmad added 2 commits December 7, 2016 18:06

added prefer related apps

39a7a45

Merge branch 'app_install_banner' of github.com:naveed-ahmad/quran.co…

8e5fbef

…m-frontend into app_install_banner

thabti merged commit bc4775f into quran:master Dec 8, 2016

App install banner #427 #508

App install banner #427 #508

Conversation

naveed-ahmad commented Nov 25, 2016

ahmedre commented Nov 25, 2016

redhae commented Nov 25, 2016 via email

naveed-ahmad commented Nov 25, 2016 • edited Loading

ahmedre commented Nov 25, 2016

thabti left a comment

Choose a reason for hiding this comment

thabti commented Nov 26, 2016

naveed-ahmad commented Nov 26, 2016

ahmedre commented Nov 26, 2016

ar5had Nov 26, 2016

Choose a reason for hiding this comment

ar5had Nov 26, 2016

Choose a reason for hiding this comment

ar5had commented Nov 27, 2016 • edited Loading

ahmedre commented Nov 27, 2016

ahmedre commented Nov 27, 2016

ahmedre commented Nov 27, 2016

naveed-ahmad commented Nov 27, 2016

ar5had commented Nov 28, 2016

ahmedre commented Dec 1, 2016

naveed-ahmad commented Dec 1, 2016

ahmedre commented Dec 1, 2016

ahmedre commented Dec 1, 2016

ahmedre commented Dec 1, 2016

ahmedre commented Dec 1, 2016

naveed-ahmad commented Dec 1, 2016

ahmedre commented Dec 1, 2016

naveed-ahmad commented Dec 1, 2016

mmahalwy commented Dec 5, 2016

naveed-ahmad commented Dec 5, 2016

ahmedre commented Dec 7, 2016

ahmedre commented Dec 7, 2016

mmahalwy commented Dec 7, 2016

naveed-ahmad commented Dec 7, 2016

ahmedre commented Dec 7, 2016

ahmedre commented Dec 7, 2016

ahmedre commented Dec 7, 2016

ahmedre commented Dec 7, 2016

mmahalwy commented Dec 7, 2016

naveed-ahmad commented Dec 8, 2016

naveed-ahmad commented Nov 25, 2016 •

edited

Loading

ar5had commented Nov 27, 2016 •

edited

Loading