Issue 662 - Job links shouldn't use anchors

Related ticket:
https://issues.adblockplus.org/ticket/662

[saroyanm, 2015-03-17 16:01:56]

Sebastian can you please have a look, when you have time.

[Sebastian Noack, 2015-03-17 16:13:32]

It would have been easier to review, if you would have used "hg mv" instead
removing/adding the job openings.

http://codereview.adblockplus.org/5994668214976512/diff/5629499534213120/page...
File pages/jobs/account-manager-usa-gb.md (right):

http://codereview.adblockplus.org/5994668214976512/diff/5629499534213120/page...
pages/jobs/account-manager-usa-gb.md:5: ## Account Manager USA/GB
According to Wladimir this duplication is unneeded. Supposedly it is possible to
output the title variable here.

http://codereview.adblockplus.org/5994668214976512/diff/5629499534213120/page...
File pages/jobs/not-available.md (right):

http://codereview.adblockplus.org/5994668214976512/diff/5629499534213120/page...
pages/jobs/not-available.md:1: title=Not available
How about "Job not available" or "Job doesn't exist"?

http://codereview.adblockplus.org/5994668214976512/diff/5629499534213120/page...
pages/jobs/not-available.md:5: ## The job you are looking for isn't available
anymore.
Did you check back with Maren which text and title to show if the job isn't
available anymore?

[saroyanm, 2015-03-17 18:12:36]

On 2015/03/17 16:13:32, Sebastian Noack wrote:
> It would have been easier to review, if you would have used "hg mv" instead
The patch is updated.

http://codereview.adblockplus.org/5994668214976512/diff/5629499534213120/page...
File pages/jobs/account-manager-usa-gb.md (right):

http://codereview.adblockplus.org/5994668214976512/diff/5629499534213120/page...
pages/jobs/account-manager-usa-gb.md:5: ## Account Manager USA/GB
On 2015/03/17 16:13:32, Sebastian Noack wrote:
> According to Wladimir this duplication is unneeded. Supposedly it is possible
to
> output the title variable here.

Hmm, not sure how it would be possible, I'm using JavaScript to highlight
selected menu item. 
@Wladimir can you please comment here ?

http://codereview.adblockplus.org/5994668214976512/diff/5629499534213120/page...
File pages/jobs/not-available.md (right):

http://codereview.adblockplus.org/5994668214976512/diff/5629499534213120/page...
pages/jobs/not-available.md:1: title=Not available
On 2015/03/17 16:13:32, Sebastian Noack wrote:
> How about "Job not available" or "Job doesn't exist"?

Done.

http://codereview.adblockplus.org/5994668214976512/diff/5629499534213120/page...
pages/jobs/not-available.md:5: ## The job you are looking for isn't available
anymore.
On 2015/03/17 16:13:32, Sebastian Noack wrote:
> Did you check back with Maren which text and title to show if the job isn't
> available anymore?

Thanks for pointing out, just aligned and updated accordingly.

[Wladimir Palant, 2015-03-18 16:54:06]

On 2015/03/17 18:12:36, saroyanm wrote:
> On 2015/03/17 16:13:32, Sebastian Noack wrote:
> > It would have been easier to review, if you would have used "hg mv" instead
> The patch is updated.

No, you are still deleting files in one place and creating new files in another
- using "hg mv" will make sure the diff only shows actual changes (in addition
to preserving file history). Please redo this, otherwise I cannot do a proper
review.

[Wladimir Palant, 2015-03-18 16:54:13]

http://codereview.adblockplus.org/5994668214976512/diff/5747976207073280/incl...
File includes/jobs/generic-header.tmpl (right):

http://codereview.adblockplus.org/5994668214976512/diff/5747976207073280/incl...
includes/jobs/generic-header.tmpl:1: {#
Frankly, it would be great if you could use |hg mv| when renaming files - then I
wouldn't have to figure out what really changed here...

http://codereview.adblockplus.org/5994668214976512/diff/5747976207073280/incl...
includes/jobs/generic-header.tmpl:24: jQuery("#" +
filename).addClass("ui-tabs-active");
Please don't do this dynamically - you can just set it statically below:

  <li id="{{ file }}"{% if page == file %} 
class="ui-tabs-active"{% endif %}>

http://codereview.adblockplus.org/5994668214976512/diff/5747976207073280/page...
File pages/jobs.tmpl (right):

http://codereview.adblockplus.org/5994668214976512/diff/5747976207073280/page...
pages/jobs.tmpl:3: <head>
This file should be called pages/jobs/index.tmpl - while the current title might
work with the test server, it will conflict with the directory name when using
the static page generator.

http://codereview.adblockplus.org/5994668214976512/diff/5747976207073280/page...
pages/jobs.tmpl:12: </script>
None of this should be necessary any more, you aren't using the tabs
functionality.

http://codereview.adblockplus.org/5994668214976512/diff/5747976207073280/page...
File pages/jobs/account-manager-usa-gb.md (right):

http://codereview.adblockplus.org/5994668214976512/diff/5747976207073280/page...
pages/jobs/account-manager-usa-gb.md:1: title=Account Manager USA/GB
Why is this duplicated here? Can't the generic-header template do {% set
title="" %}? Also, it should be able to generate the header below.

[saroyanm, 2015-03-18 17:40:35]

On 2015/03/18 16:54:06, Wladimir Palant wrote:
> On 2015/03/17 18:12:36, saroyanm wrote:
> > On 2015/03/17 16:13:32, Sebastian Noack wrote:
> > > It would have been easier to review, if you would have used "hg mv"
instead
> > The patch is updated.
> 
> No, you are still deleting files in one place and creating new files in
another
> - using "hg mv" will make sure the diff only shows actual changes (in addition
> to preserving file history). Please redo this, otherwise I cannot do a proper
> review.

I've used hg mv to move current openings we have and removed the one which are
not available anymore, The only case I can see where I didn't used `hg mv` is
with generic-header.tmpl.
Hope I don't missing something.

[Wladimir Palant, 2015-03-18 18:08:05]

On 2015/03/18 17:40:35, saroyanm wrote:
> I've used hg mv to move current openings we have and removed the one which are
> not available anymore,

Looking at the raw patch - yes, that's what you did. It seems that the massive
change was rather due to a change of line endings. Could you make sure to keep
Windows-style line endings in the files that were using them? We can clean up
afterwards.

[saroyanm, 2015-03-19 17:37:29]

On 2015/03/18 18:08:05, Wladimir Palant wrote:
> On 2015/03/18 17:40:35, saroyanm wrote:
> > I've used hg mv to move current openings we have and removed the one which
are
> > not available anymore,
> 
> Looking at the raw patch - yes, that's what you did. It seems that the massive
> change was rather due to a change of line endings. Could you make sure to keep
> Windows-style line endings in the files that were using them? We can clean up
> afterwards.

Sorry for late answer, 
Lost a lot of time in figuring out what's wrong with the downloaded raw patch,
seems like raw patch always convert all endings into Unix one :/
Exported patch and downloaded patch from rietvield are different. 
I'll send you the patch by Email. Hope it will help.

[saroyanm, 2015-03-19 17:37:44]

http://codereview.adblockplus.org/5994668214976512/diff/5747976207073280/incl...
File includes/jobs/generic-header.tmpl (right):

http://codereview.adblockplus.org/5994668214976512/diff/5747976207073280/incl...
includes/jobs/generic-header.tmpl:1: {#
On 2015/03/18 16:54:13, Wladimir Palant wrote:
> Frankly, it would be great if you could use |hg mv| when renaming files - then
I
> wouldn't have to figure out what really changed here...

Good point, thanks.

http://codereview.adblockplus.org/5994668214976512/diff/5747976207073280/incl...
includes/jobs/generic-header.tmpl:24: jQuery("#" +
filename).addClass("ui-tabs-active");
On 2015/03/18 16:54:13, Wladimir Palant wrote:
> Please don't do this dynamically - you can just set it statically below:
> 
>   <li id="{{ file }}"{% if page == file %} 
> class="ui-tabs-active"{% endif %}>

Done.

http://codereview.adblockplus.org/5994668214976512/diff/5747976207073280/page...
File pages/jobs.tmpl (right):

http://codereview.adblockplus.org/5994668214976512/diff/5747976207073280/page...
pages/jobs.tmpl:3: <head>
On 2015/03/18 16:54:13, Wladimir Palant wrote:
> This file should be called pages/jobs/index.tmpl - while the current title
might
> work with the test server, it will conflict with the directory name when using
> the static page generator.

Done.

http://codereview.adblockplus.org/5994668214976512/diff/5747976207073280/page...
pages/jobs.tmpl:12: </script>
On 2015/03/18 16:54:13, Wladimir Palant wrote:
> None of this should be necessary any more, you aren't using the tabs
> functionality.

Yes this is the overview page and it contains some tabs that are using jquery:
"Why work here"
"Employe benefits"
"Our Office"

http://codereview.adblockplus.org/5994668214976512/diff/5747976207073280/page...
File pages/jobs/account-manager-usa-gb.md (right):

http://codereview.adblockplus.org/5994668214976512/diff/5747976207073280/page...
pages/jobs/account-manager-usa-gb.md:1: title=Account Manager USA/GB
On 2015/03/18 16:54:13, Wladimir Palant wrote:
> Why is this duplicated here? Can't the generic-header template do {% set
> title="" %}? Also, it should be able to generate the header below.

I have assumption that the title variable is not being available in included
file. 
Didn't had success in setting title variable inside generic-header, but updated
the default template to be able to use head tag for that purpose, please let me
know whether current solution is Okey.

[saroyanm, 2015-03-20 10:00:03]

Additional information regarding last uploaded patch.
Wladimir Changed line endings to UNIX one and removed the openings that are not
available anymore:
https://hg.adblockplus.org/web.eyeo.com/rev/39bc9054f85c
https://hg.adblockplus.org/web.eyeo.com/rev/7ba6c84d0d06

Also added some blocking tickets to current one:
#2178, #2179, #2180, #2181

[saroyanm, 2015-03-20 16:49:52]

Uploaded another patch by Wladimir. 
Will have a closer look over the weekend.

[Sebastian Noack, 2015-03-20 17:00:52]

http://codereview.adblockplus.org/5994668214976512/diff/5050731216240640/incl...
File includes/jobs/generic-header.tmpl (right):

http://codereview.adblockplus.org/5994668214976512/diff/5050731216240640/incl...
includes/jobs/generic-header.tmpl:18: {% set title, _ =
openings|find(page.split("/")[-1], attribute=1) or (title, "") %}
I'd rather move the whole logic into a function like get_title(page, openings).
This is a little much logic in the template for my taste, in particular if you
added already a filter for this particular use case.

http://codereview.adblockplus.org/5994668214976512/diff/5050731216240640/page...
File pages/jobs/not-available.md (right):

http://codereview.adblockplus.org/5994668214976512/diff/5050731216240640/page...
pages/jobs/not-available.md:1: title=Job not available
I wonder whether this clashes with the logic setting the title in
generic-header.tmpl.

[Wladimir Palant, 2015-03-20 17:35:52]

http://codereview.adblockplus.org/5994668214976512/diff/5050731216240640/incl...
File includes/jobs/generic-header.tmpl (right):

http://codereview.adblockplus.org/5994668214976512/diff/5050731216240640/incl...
includes/jobs/generic-header.tmpl:18: {% set title, _ =
openings|find(page.split("/")[-1], attribute=1) or (title, "") %}
On 2015/03/20 17:00:52, Sebastian Noack wrote:
> I'd rather move the whole logic into a function like get_title(page,
openings).
> This is a little much logic in the template for my taste, in particular if you
> added already a filter for this particular use case.

find() as it is currently defined is a pretty generic filter. get_title()
function on the other hand would have to consider specifics of the data
structure used in this particular template. All that would achieve is breaking
code locality and spreading out logic that belongs together.

http://codereview.adblockplus.org/5994668214976512/diff/5050731216240640/incl...
includes/jobs/generic-header.tmpl:18: {% set title, _ =
openings|find(page.split("/")[-1], attribute=1) or (title, "") %}
How about {% set opening = openings|find(page.split("/")[-1], attribute=1) %}
here and {% set title, _ = opening or (title, "") %}? The opening variable could
be reused in the footer to add the "apply now" button conditionally - you would
no longer need two footers.

http://codereview.adblockplus.org/5994668214976512/diff/5050731216240640/incl...
includes/jobs/generic-header.tmpl:31: <li {% if "jobs/" ~ file == page %}
class="ui-tabs-active" {% endif %}>
I really wouldn't hardcode the full page path here. Maybe {% if file ==
page.split("/")[-1] %} like above?

http://codereview.adblockplus.org/5994668214976512/diff/5050731216240640/incl...
includes/jobs/generic-header.tmpl:32: <a href="/en/jobs/{{ file }}"
class="ui-tabs-anchor">
This should be href="{{("jobs/" + file)|linkify}}"

http://codereview.adblockplus.org/5994668214976512/diff/5050731216240640/incl...
File includes/jobs/opening-footer.tmpl (right):

http://codereview.adblockplus.org/5994668214976512/diff/5050731216240640/incl...
includes/jobs/opening-footer.tmpl:2: <a href="/en/jobs"><button
class="why-work-here-button">why work here?</button></a>
href="{{"jobs"|linkify}}" please.

http://codereview.adblockplus.org/5994668214976512/diff/5050731216240640/page...
File pages/jobs/not-available.md (right):

http://codereview.adblockplus.org/5994668214976512/diff/5050731216240640/page...
pages/jobs/not-available.md:1: title=Job not available
On 2015/03/20 17:00:52, Sebastian Noack wrote:
> I wonder whether this clashes with the logic setting the title in
> generic-header.tmpl.

It doesn't because "not-available" isn't in the list of job openings.

[Sebastian Noack, 2015-03-20 17:50:28]

http://codereview.adblockplus.org/5994668214976512/diff/5050731216240640/filt...
File filters/find.py (right):

http://codereview.adblockplus.org/5994668214976512/diff/5050731216240640/filt...
filters/find.py:4: (attribute is not None and item[attribute] == value)):
If you insist on keeping this weird filter, can we please simplify the logic at
least?

(item if attribute is None else item[attribute]) == value

http://codereview.adblockplus.org/5994668214976512/diff/5050731216240640/incl...
File includes/jobs/generic-header.tmpl (right):

http://codereview.adblockplus.org/5994668214976512/diff/5050731216240640/incl...
includes/jobs/generic-header.tmpl:18: {% set title, _ =
openings|find(page.split("/")[-1], attribute=1) or (title, "") %}
On 2015/03/20 17:35:52, Wladimir Palant wrote:
> How about {% set opening = openings|find(page.split("/")[-1], attribute=1) %}
> here and {% set title, _ = opening or (title, "") %}? The opening variable
could
> be reused in the footer to add the "apply now" button conditionally - you
would
> no longer need two footers.

This would make the logic certainly clearer. Still not that great IMO.

[Wladimir Palant, 2015-03-20 18:22:40]

http://codereview.adblockplus.org/5994668214976512/diff/5050731216240640/filt...
File filters/find.py (right):

http://codereview.adblockplus.org/5994668214976512/diff/5050731216240640/filt...
filters/find.py:4: (attribute is not None and item[attribute] == value)):
On 2015/03/20 17:50:28, Sebastian Noack wrote:
> If you insist on keeping this weird filter, can we please simplify the logic
at
> least?
> 
> (item if attribute is None else item[attribute]) == value

Not that much simpler but fine with me.

[Sebastian Noack, 2015-03-21 12:34:43]

http://codereview.adblockplus.org/5994668214976512/diff/5050731216240640/filt...
File filters/find.py (right):

http://codereview.adblockplus.org/5994668214976512/diff/5050731216240640/filt...
filters/find.py:4: (attribute is not None and item[attribute] == value)):
On 2015/03/20 18:22:40, Wladimir Palant wrote:
> On 2015/03/20 17:50:28, Sebastian Noack wrote:
> > If you insist on keeping this weird filter, can we please simplify the logic
> at
> > least?
> > 
> > (item if attribute is None else item[attribute]) == value
> 
> Not that much simpler but fine with me.

Moreover, you should use environment.getitem(). Otherwise only items, but no
attributes will be considered.

@environmentfilter
def find(environment, sequence, value, attr=None)
  for item in sequence:
    if (item if attr is None else environment.getitem(item, attr)) == value:
      return item
  return None

[saroyanm, 2015-03-21 17:27:10]

New patch uploaded.

http://codereview.adblockplus.org/5994668214976512/diff/5050731216240640/filt...
File filters/find.py (right):

http://codereview.adblockplus.org/5994668214976512/diff/5050731216240640/filt...
filters/find.py:4: (attribute is not None and item[attribute] == value)):
On 2015/03/21 12:34:44, Sebastian Noack wrote:
> On 2015/03/20 18:22:40, Wladimir Palant wrote:
> > On 2015/03/20 17:50:28, Sebastian Noack wrote:
> > > If you insist on keeping this weird filter, can we please simplify the
logic
> > at
> > > least?
> > > 
> > > (item if attribute is None else item[attribute]) == value
> > 
> > Not that much simpler but fine with me.
> 
> Moreover, you should use environment.getitem(). Otherwise only items, but no
> attributes will be considered.
> 
> @environmentfilter
> def find(environment, sequence, value, attr=None)
>   for item in sequence:
>     if (item if attr is None else environment.getitem(item, attr)) == value:
>       return item
>   return None

Done.

http://codereview.adblockplus.org/5994668214976512/diff/5050731216240640/incl...
File includes/jobs/generic-header.tmpl (right):

http://codereview.adblockplus.org/5994668214976512/diff/5050731216240640/incl...
includes/jobs/generic-header.tmpl:18: {% set title, _ =
openings|find(page.split("/")[-1], attribute=1) or (title, "") %}
On 2015/03/20 17:50:28, Sebastian Noack wrote:
> On 2015/03/20 17:35:52, Wladimir Palant wrote:
> > How about {% set opening = openings|find(page.split("/")[-1], attribute=1)
%}
> > here and {% set title, _ = opening or (title, "") %}? The opening variable
> could
> > be reused in the footer to add the "apply now" button conditionally - you
> would
> > no longer need two footers.
> 
> This would make the logic certainly clearer. Still not that great IMO.

Done.

http://codereview.adblockplus.org/5994668214976512/diff/5050731216240640/incl...
includes/jobs/generic-header.tmpl:31: <li {% if "jobs/" ~ file == page %}
class="ui-tabs-active" {% endif %}>
On 2015/03/20 17:35:52, Wladimir Palant wrote:
> I really wouldn't hardcode the full page path here. Maybe {% if file ==
> page.split("/")[-1] %} like above?

Done.

http://codereview.adblockplus.org/5994668214976512/diff/5050731216240640/incl...
includes/jobs/generic-header.tmpl:32: <a href="/en/jobs/{{ file }}"
class="ui-tabs-anchor">
On 2015/03/20 17:35:52, Wladimir Palant wrote:
> This should be href="{{("jobs/" + file)|linkify}}"

Done.

http://codereview.adblockplus.org/5994668214976512/diff/5050731216240640/incl...
File includes/jobs/opening-footer.tmpl (right):

http://codereview.adblockplus.org/5994668214976512/diff/5050731216240640/incl...
includes/jobs/opening-footer.tmpl:2: <a href="/en/jobs"><button
class="why-work-here-button">why work here?</button></a>
On 2015/03/20 17:35:52, Wladimir Palant wrote:
> href="{{"jobs"|linkify}}" please.

Done.

[Wladimir Palant, 2015-03-22 22:06:42]

http://codereview.adblockplus.org/5994668214976512/diff/5050731216240640/filt...
File filters/find.py (right):

http://codereview.adblockplus.org/5994668214976512/diff/5050731216240640/filt...
filters/find.py:4: (attribute is not None and item[attribute] == value)):
On 2015/03/21 12:34:44, Sebastian Noack wrote:
> Moreover, you should use environment.getitem(). Otherwise only items, but no
> attributes will be considered.

I doubt that this is a good idea. Environment.getitem() isn't documented - it's
an internal utility function that we shouldn't rely on unless we want to risk
breakage with a future Jinja2 version. Not sure whether duplicating getattr()
from Jinja2 is worth it but at least it would be a reliable solution without a
bogus dependency on the Jinja2 environment:

  def getattribute(obj, attribute):
    try:
      return getattr(obj, attribute)
    except AttributeError:
      return obj[attribute]

http://codereview.adblockplus.org/5994668214976512/diff/4789962276864000/filt...
File filters/find.py (right):

http://codereview.adblockplus.org/5994668214976512/diff/4789962276864000/filt...
filters/find.py:1: from jinja2 import environmentfilter, Environment
Nit: Environment isn't used. Not that it really matters (see my other comment).

http://codereview.adblockplus.org/5994668214976512/diff/4789962276864000/filt...
filters/find.py:4: def find(environment, sequence, value, attr=None):
Please keep attribute as the parameter name here, this would be consistent with
other template filters
(http://jinja.pocoo.org/docs/dev/templates/#builtin-filters).

http://codereview.adblockplus.org/5994668214976512/diff/4789962276864000/incl...
File includes/jobs/generic-footer.tmpl (right):

http://codereview.adblockplus.org/5994668214976512/diff/4789962276864000/incl...
includes/jobs/generic-footer.tmpl:1: {% if opening %}
Nit: Rename this file into footer.tmpl? Same with the header of course.

http://codereview.adblockplus.org/5994668214976512/diff/4789962276864000/incl...
File includes/jobs/generic-header.tmpl (right):

http://codereview.adblockplus.org/5994668214976512/diff/4789962276864000/incl...
includes/jobs/generic-header.tmpl:33: {{("jobs/" + file)|linkify(class =
"ui-tabs-anchor")}}
Nit: We generally don't put spaces around = for named parameters, only
assignments. This matches PEP 8 recommendations:

> Don't use spaces around the = sign when used to indicate a keyword argument or
a default parameter value.

[saroyanm, 2015-03-22 23:40:50]

Patch updated.

http://codereview.adblockplus.org/5994668214976512/diff/5050731216240640/filt...
File filters/find.py (right):

http://codereview.adblockplus.org/5994668214976512/diff/5050731216240640/filt...
filters/find.py:4: (attribute is not None and item[attribute] == value)):
On 2015/03/22 22:06:42, Wladimir Palant wrote:
> On 2015/03/21 12:34:44, Sebastian Noack wrote:
> > Moreover, you should use environment.getitem(). Otherwise only items, but no
> > attributes will be considered.
> 
> I doubt that this is a good idea. Environment.getitem() isn't documented -
it's
> an internal utility function that we shouldn't rely on unless we want to risk
> breakage with a future Jinja2 version. Not sure whether duplicating getattr()
> from Jinja2 is worth it but at least it would be a reliable solution without a
> bogus dependency on the Jinja2 environment:
> 
>   def getattribute(obj, attribute):
>     try:
>       return getattr(obj, attribute)
>     except AttributeError:
>       return obj[attribute]

Done.

http://codereview.adblockplus.org/5994668214976512/diff/4789962276864000/filt...
File filters/find.py (right):

http://codereview.adblockplus.org/5994668214976512/diff/4789962276864000/filt...
filters/find.py:4: def find(environment, sequence, value, attr=None):
On 2015/03/22 22:06:42, Wladimir Palant wrote:
> Please keep attribute as the parameter name here, this would be consistent
with
> other template filters
> (http://jinja.pocoo.org/docs/dev/templates/#builtin-filters).

Done.

http://codereview.adblockplus.org/5994668214976512/diff/4789962276864000/incl...
File includes/jobs/generic-footer.tmpl (right):

http://codereview.adblockplus.org/5994668214976512/diff/4789962276864000/incl...
includes/jobs/generic-footer.tmpl:1: {% if opening %}
On 2015/03/22 22:06:42, Wladimir Palant wrote:
> Nit: Rename this file into footer.tmpl? Same with the header of course.

Done.

http://codereview.adblockplus.org/5994668214976512/diff/4789962276864000/incl...
File includes/jobs/generic-header.tmpl (right):

http://codereview.adblockplus.org/5994668214976512/diff/4789962276864000/incl...
includes/jobs/generic-header.tmpl:33: {{("jobs/" + file)|linkify(class =
"ui-tabs-anchor")}}
On 2015/03/22 22:06:42, Wladimir Palant wrote:
> Nit: We generally don't put spaces around = for named parameters, only
> assignments. This matches PEP 8 recommendations:
> 
> > Don't use spaces around the = sign when used to indicate a keyword argument
or
> a default parameter value.

Thanks for reference, done

http://codereview.adblockplus.org/5994668214976512/diff/6204636457336832/temp...
File templates/default.tmpl (right):

http://codereview.adblockplus.org/5994668214976512/diff/6204636457336832/temp...
templates/default.tmpl:11: <title>Eyeo GmbH</title>
I think make sense to also consider the else case, BTW is there a reason not to
use indentation in head tag ?

[saroyanm, 2015-03-22 23:45:26]

A small nit fixed.

[Sebastian Noack, 2015-03-23 10:45:21]

http://codereview.adblockplus.org/5994668214976512/diff/6012694368878592/filt...
File filters/find.py (right):

http://codereview.adblockplus.org/5994668214976512/diff/6012694368878592/filt...
filters/find.py:1: def getattribute(obj, attribute):
Why don't you use environment.getitem() anymore?

http://codereview.adblockplus.org/5994668214976512/diff/6012694368878592/page...
File pages/jobs/index.tmpl (right):

http://codereview.adblockplus.org/5994668214976512/diff/6012694368878592/page...
pages/jobs/index.tmpl:8: jQuery(function()
Now, where every job opening is an own page, I think we don't need JavaScript
for the tabs anymore, but can use only CSS, right?

[saroyanm, 2015-03-23 10:59:13]

http://codereview.adblockplus.org/5994668214976512/diff/6012694368878592/filt...
File filters/find.py (right):

http://codereview.adblockplus.org/5994668214976512/diff/6012694368878592/filt...
filters/find.py:1: def getattribute(obj, attribute):
On 2015/03/23 10:45:21, Sebastian Noack wrote:
> Why don't you use environment.getitem() anymore?

I've changed according to the last Reply here:
http://codereview.adblockplus.org/5994668214976512/diff/5050731216240640/filt...
Please let me know if you still think that we need to use environment.getitem()

http://codereview.adblockplus.org/5994668214976512/diff/6012694368878592/page...
File pages/jobs/index.tmpl (right):

http://codereview.adblockplus.org/5994668214976512/diff/6012694368878592/page...
pages/jobs/index.tmpl:8: jQuery(function()
On 2015/03/23 10:45:21, Sebastian Noack wrote:
> Now, where every job opening is an own page, I think we don't need JavaScript
> for the tabs anymore, but can use only CSS, right?

Actually we have tabs on this page that rely on Jquery:
* Why work with us
* Employee benefits
* Our office

In general I agree that it make sense to Get rid of JQuery and use CSS and
Javascript, but I think It should be separate issue, while we have JQuery tabs
also on other pages.

[Sebastian Noack, 2015-03-23 11:19:58]

http://codereview.adblockplus.org/5994668214976512/diff/6012694368878592/filt...
File filters/find.py (right):

http://codereview.adblockplus.org/5994668214976512/diff/6012694368878592/filt...
filters/find.py:1: def getattribute(obj, attribute):
On 2015/03/23 10:59:13, saroyanm wrote:
> On 2015/03/23 10:45:21, Sebastian Noack wrote:
> > Why don't you use environment.getitem() anymore?
> 
> I've changed according to the last Reply here:
>
http://codereview.adblockplus.org/5994668214976512/diff/5050731216240640/filt...
> Please let me know if you still think that we need to use
environment.getitem()

Sorry, I missed that comment. However, I don't agree with Wladimir. This method
exists to assist (custom) filters and functions. Neither is it marked as
internal, unlike other methods (starting with an underscore). It is actually
documented in the source code, however not in the online documentation, which I
can only assume to be a bug.

[Sebastian Noack, 2015-03-23 12:31:14]

http://codereview.adblockplus.org/5994668214976512/diff/6012694368878592/filt...
File filters/find.py (right):

http://codereview.adblockplus.org/5994668214976512/diff/6012694368878592/filt...
filters/find.py:1: def getattribute(obj, attribute):
For reference:

13:15:01 <snoack> mitsuhiko: Is Environment.getitem() and Environment.getattr()
supposed to be an internal API, or is it safe to be used in custom filters?
13:15:19 <mitsuhiko> it's public
13:16:39 <snoack> mitsuhiko: Any reason it's not documented (except for the
source code) then? 
13:17:44 <mitsuhiko> snoack: discouragement
13:17:49 <mitsuhiko> same as with template_class i think
13:18:15 <mitsuhiko> it's public api to support the sandbox though
13:18:58 <mitsuhiko> since filters started calling it i might just document it

[saroyanm, 2015-03-23 13:07:49]

patch updated.

http://codereview.adblockplus.org/5994668214976512/diff/6012694368878592/filt...
File filters/find.py (right):

http://codereview.adblockplus.org/5994668214976512/diff/6012694368878592/filt...
filters/find.py:1: def getattribute(obj, attribute):
On 2015/03/23 11:19:59, Sebastian Noack wrote:
> On 2015/03/23 10:59:13, saroyanm wrote:
> > On 2015/03/23 10:45:21, Sebastian Noack wrote:
> > > Why don't you use environment.getitem() anymore?
> > 
> > I've changed according to the last Reply here:
> >
>
http://codereview.adblockplus.org/5994668214976512/diff/5050731216240640/filt...
> > Please let me know if you still think that we need to use
> environment.getitem()
> 
> Sorry, I missed that comment. However, I don't agree with Wladimir. This
method
> exists to assist (custom) filters and functions. Neither is it marked as
> internal, unlike other methods (starting with an underscore). It is actually
> documented in the source code, however not in the online documentation, which
I
> can only assume to be a bug.

Done.

[Sebastian Noack, 2015-03-23 13:21:34]

LGTM

[saroyanm, 2015-03-23 13:35:47]

On 2015/03/23 13:21:34, Sebastian Noack wrote:
> LGTM

@Wladimir can you please give me access to `web.eyeo.com` repository so I can
push my changes ?

[Wladimir Palant, 2015-03-23 14:07:01]

LGTM