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

Add support to embed hasjob iframe on any third party website #455

Merged
merged 8 commits into from
Jun 21, 2018

Conversation

vidya-ram
Copy link
Member

@vidya-ram vidya-ram commented Jun 13, 2018

  1. JS for adding iframe to third party website and to resize the iframe
    automatically to match content size
  2. iframe endpoint, accessed by passing ?embed=1 to any view URL,
    plus &limit=n where default is 8 when an embed is specified
  3. In iframe view, grouping is disabled
  4. Clicking on job posts opens Hasjob on a new tab

To add iframe to a website, include container div and script tag

<div class="hasjob-embed" data-href="http://hasjob.co" data-jobpost-limit="16"></div>
<script src="https://hasjob.co/api/1/embed.js"></script>

Fixes #454

Desktop view

Mobile view(on resizing)

1. JS for adding iframe to third party website and to resize the iframe
automatically to match content size
2. iframe endpoint, accessed by passing `?embed=1` to any view URL,
plus `&limit=n` where default is 8 when an embed is specified
3. In iframe view, grouping is disabled
@vidya-ram vidya-ram requested review from jace and iambibhas June 13, 2018 14:37
@@ -0,0 +1,24 @@
(function() {
var containerDiv = document.getElementById('hasjob');
Copy link
Member

Choose a reason for hiding this comment

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

This assumes there can be only one Hasjob embed per page. Do we want to support multiple? In the case of a Medium article, for instance, there can be multiple. While this code isn't for Medium, we may have a similar use case on our own websites.

Copy link
Member Author

@vidya-ram vidya-ram Jun 15, 2018

Choose a reason for hiding this comment

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

Fixed.
To include multiple hasjob iframes, container div needs to have
data-href : hasjob url (can contain query parameters)
data-jobpost-limit: Number of job posts to be displayed
data-iframe-id: id for the iframe (this is required to set the height of the respective iframe on resize)

Eg:-

<div class="hasjob-embed" data-href="http://hasjob.co/?q=javascript" data-jobpost-limit="16" data-iframe-id="hasjob-js"></div>
<div class="hasjob-embed" data-href="http://hasjob.co/?t=contract&q=android" data-jobpost-limit="16" data-iframe-id="hasjob-android"></div>
<script src="http://hasjob.co/api/1/embed.js"></script>

var link = containerDiv.getAttribute('data-href');
link = link.substr(-1) !== '/' ? link : link.substr(0, link.length-1);
var jobPostLimit = containerDiv.getAttribute('data-jobpost-limit');
var iframeSrc = link + '/?embed=1&limit=' + jobPostLimit;
Copy link
Member

Choose a reason for hiding this comment

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

This won't work if the link already contains query parameters. Have to check for that.

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

{{ gboard.description|safe }}
{%- if gkiosk and g.peopleflow_url %}
<p>If you find a job worth applying for here, tap your badge on the reader attached to this kiosk and we’ll send an email connecting you with the employer.</p>
{%- if not embed %}
Copy link
Member

Choose a reason for hiding this comment

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

Due to the complexity of this template, I'm unable to tell if the if/endif tags are correctly positioned and have to trust you on this. We should simplify this in a separate PR, composing the template as a collection of calls to macros.

Copy link
Member Author

Choose a reason for hiding this comment

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

Moved stickie-area rendering to a macro

{%- if not embed %}
{{ super() }}
{% endif %}
{% endblock %}
Copy link
Member

Choose a reason for hiding this comment

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

Instead of this, make an embedlayout.html.jinja2 and in index.html.jinja2, add an if condition to decide which template to extend.

@@ -702,7 +713,7 @@ def sortarchive(order_by):
if reverse is None:
reverse = False
try:
reverse = bool(int(reverse))
reverse = bool( (reverse))
Copy link
Member

Choose a reason for hiding this comment

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

?

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('/embed.js', methods=['GET'], subdomain='<subdomain>')
@app.route('/embed.js', methods=['GET'])
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 /api/1/embed.js

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

<!--[if lt IE 7]> <html class="no-js lt-ie9 lt-ie8 lt-ie7" lang="en" {% block manifest %}{%- if g.kiosk %} manifest="{{ url_for('kiosk_manifest') }}" {%- endif %}{% endblock %}> <![endif]-->
<!--[if IE 7]> <html class="no-js lt-ie9 lt-ie8" lang="en" {{ self.manifest() }}> <![endif]-->
<!--[if IE 8]> <html class="no-js lt-ie9" lang="en" {{ self.manifest() }}> <![endif]-->
<!--[if gt IE 8]><!--> <html lang="en" class="no-js {%- if g.user %} userlogin {%- else %} no-userlogin {%- endif %}" {{ self.manifest() }}> <!--<![endif]-->
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 this manifest block into baseframe so we don't have to repeat it here.

}
}
</script>
{%- endblock %}
Copy link
Member

Choose a reason for hiding this comment

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

There is too much replicated from layout.html.jinja2 here. We'll have to find a way to abstract this out. Maybe in a separate PR.

Copy link
Member Author

@vidya-ram vidya-ram Jun 18, 2018

Choose a reason for hiding this comment

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

@jace I extended layout.html.jinja2 in embedlayout.html.jinja2

Copy link
Member

Choose a reason for hiding this comment

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

Better now!


{%- if embed -%}
{% extends "embedlayout.html.jinja2" %}
{%- elif not request.is_xhr -%}
Copy link
Member

Choose a reason for hiding this comment

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

Wait, doesn't this if condition do the same thing we're trying to achieve with embed? It returns an embeddable version of the page without header or footer, right?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes and that's what I had done earlier. There are other parts of the page that we don't need in the embed like flash, campaign, footerscripts, there was more if/endif tags and increasing the complexity of the template.

Copy link
Member Author

Choose a reason for hiding this comment

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

Also if not request.is_xhr doesn't include baseframe styling

Copy link
Member

Choose a reason for hiding this comment

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

As a clean-up project, can we design templates around these use cases?

  1. Full page
  2. iframe (no header and footer, but includes stylesheet and scripts)
  3. Fragment (just a HTML snippet)

This needs a separate ticket, and should be supported from Baseframe, so the ticket goes there.

}
}
</script>
{%- endblock %}
Copy link
Member

Choose a reason for hiding this comment

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

Better now!


{%- if embed -%}
{% extends "embedlayout.html.jinja2" %}
{%- elif not request.is_xhr -%}
Copy link
Member

Choose a reason for hiding this comment

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

As a clean-up project, can we design templates around these use cases?

  1. Full page
  2. iframe (no header and footer, but includes stylesheet and scripts)
  3. Fragment (just a HTML snippet)

This needs a separate ticket, and should be supported from Baseframe, so the ticket goes there.

@vidya-ram vidya-ram merged commit 3724393 into master Jun 21, 2018
@vidya-ram vidya-ram deleted the widget branch June 21, 2018 12:18
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.

2 participants