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

hasjob PWA #425

Merged
merged 20 commits into from
Apr 17, 2018
Merged

hasjob PWA #425

merged 20 commits into from
Apr 17, 2018

Conversation

vidya-ram
Copy link
Member

@vidya-ram vidya-ram commented Mar 7, 2018

  1. Introduced Webpack to build & bundle hasjob's css & js
  2. Used Workbox Webpack plugin to generate service worker that takes care of caching & offline support
  3. Add Manifest.json to enable "Add to homescreen" feature.
  4. Add the new Hasjob logo
    add to homescreen   icon on homescreen

if ('serviceWorker' in navigator) {
navigator.serviceWorker.register('/service-worker.js')
.then(function(registration) {
console.log('Service Worker registration successful with scope: ',
Copy link
Contributor

Choose a reason for hiding this comment

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

Perhaps better to raise an exception with Sentry?

@iambibhas
Copy link
Contributor

I really wish we didn't include built static files in the repo. Can we move the building part to the server? just include the main static files in repo and then when deploying, run build command.

ext_requires=['baseframe-bs3',
('jquery.autosize', 'jquery.sparkline', 'jquery.liblink', 'jquery.wnumb', 'jquery.nouislider'),
baseframe.init_app(app, requires=['baseframe-bs3',
'jquery.autosize', 'jquery.sparkline', 'jquery.liblink', 'jquery.wnumb', 'jquery.nouislider',
Copy link
Member

Choose a reason for hiding this comment

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

We stopped using sparkline a while ago. Safe to remove now.

Copy link
Member Author

Choose a reason for hiding this comment

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

Removed

@@ -0,0 +1 @@
import '../sass/app.sass';
Copy link
Member

Choose a reason for hiding this comment

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

If this needs compilation, it should be in the assets folder, not static.

Copy link
Member

Choose a reason for hiding this comment

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

Everything in static is published to the world and can be directly accessed from a URL.

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed

@@ -1,6 +1,6 @@
body
font-family: $font-default
background: #e8e7e6 image-url("canvas.jpg")
background: #e8e7e6 url("~/static/img/canvas.jpg")
Copy link
Member

Choose a reason for hiding this comment

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

We should move SASS files into assets as well, which means this path will need to be updated (image files stay in static).

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed

@@ -12,7 +12,7 @@
</div>
{% endblock %}

{% block footerscripts %}
{% block tablelayoutscripts %}
Copy link
Member

Choose a reason for hiding this comment

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

This should be tablayout, not tablelayout. Please amend in the source template as well.

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed

@@ -229,5 +240,20 @@

{% block layoutscripts %}
{%- if header_campaign %}{{ campaign_script() }}{% endif %}
<script src='{{ asset_path('manifest') | safe }}' type="text/javascript"></script>
<script src='{{ asset_path('vendor') | safe }}' type="text/javascript"></script>
<script src='{{ asset_path('app') | safe }}' type="text/javascript"></script>
Copy link
Member

Choose a reason for hiding this comment

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

Not safe to use the | safe filter. If asset_path is confirming the strings are escaped, let it return Markup strings.

Copy link
Member Author

Choose a reason for hiding this comment

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

Changed

@@ -21,7 +21,7 @@
</div>
</div>
{% endblock %}
{% block footerscripts %}
{% block sheetscripts %}
Copy link
Member

Choose a reason for hiding this comment

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

These templates should continue to use footerscripts. Only overwrite sheetscripts or tablayoutscripts in layout templates.

Copy link
Member Author

Choose a reason for hiding this comment

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

In stats.html.jinja
{% block sheetscripts %}
{% block footerscripts %}
.....
{% endblock %}
{% endblock %}
Would this be fine, @jace ?

Copy link
Member

Choose a reason for hiding this comment

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

Using just {% block footerscripts %} should work.

Copy link
Member Author

Choose a reason for hiding this comment

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

In sheet.html.jinja2, I have added
{% block layoutscripts %}
{{ super() }}
{% endblock %}
And used just {% block footerscripts %} in templates that extend sheet.html.jinja2

<script type="text/javascript" src="//cdnjs.cloudflare.com/ajax/libs/d3/3.5.17/d3.min.js"></script>
<script type="text/javascript" src="//cdnjs.cloudflare.com/ajax/libs/c3/0.4.14/c3.min.js"></script>
{% block footerscripts %}{% endblock %}
{% endblock %}
{% block tablelayoutscripts %}
Copy link
Member

Choose a reason for hiding this comment

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

{% block tablayoutscripts %}

Copy link
Member Author

Choose a reason for hiding this comment

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

fixed


@app.route('/workbox-sw.prod.v2.1.2.js', methods=['GET'])
def workbox():
return app.send_static_file('workbox-sw.prod.v2.1.2.js')
Copy link
Member

Choose a reason for hiding this comment

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

What is v2.1.2?

Copy link
Member Author

Choose a reason for hiding this comment

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

workbox-webpack-plugin generates the service worker that import's Workbox's service worker library
https://developers.google.com/web/tools/workbox/get-started/webpack#register

Copy link
Member

Choose a reason for hiding this comment

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

So v2.1.2 is a static path that won't change? Or it's linked to the webpack version? If latter, we should avoid a dependency on having to change this line every time webpack changes.

Copy link
Member Author

Choose a reason for hiding this comment

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

Removed

@@ -22,6 +22,5 @@
{% endblock %}

{% block layoutscripts %}
{%- if header_campaign %}{{ campaign_script() }}{% endif %}
{% block footerscripts %}{% endblock %}
{{ super() }}
{% endblock %}
Copy link
Member

Choose a reason for hiding this comment

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

We don't need to define a block if it only contains super().

Copy link
Member Author

Choose a reason for hiding this comment

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

Removed.

@@ -819,3 +819,18 @@ def logoimage(domain, hashid):
@app.route('/search')
def search():
return redirect(url_for('index', **request.args))


@app.route('/offline')
Copy link
Member

Choose a reason for hiding this comment

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

Can we be careful about adding new top-level routes? This merits a discussion. See #145.

Copy link
Member Author

Choose a reason for hiding this comment

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

Changed it to /api/1/template/offline

@jace
Copy link
Member

jace commented Apr 15, 2018

I have just these two comments, @vidya-ram.

@vidya-ram vidya-ram merged commit 8a2f70e into master Apr 17, 2018
@vidya-ram vidya-ram deleted the service-worker branch April 17, 2018 09:07
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.

4 participants