Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(426)

Issue 29487683: Issue 5221 - Updated jobs page text on eyeo.com (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
2 years, 7 months ago by juliandoucette
Modified:
2 years, 7 months ago
Reviewers:
ire, saroyanm
Base URL:
https://hg.adblockplus.org/web.eyeo.com
Visibility:
Public.

Description

Issue 5221 - Updated jobs page text on eyeo.com

Patch Set 1 #

Patch Set 2 : Removed unnessisary headings and renamed tab includes #

Total comments: 6

Patch Set 3 : Addressed Ire's comments #

Unified diffs Side-by-side diffs Delta from patch set Stats (+92 lines, -42 lines) Patch
M includes/jobs/employee-perks.md View 1 2 1 chunk +14 lines, -9 lines 0 comments Download
M includes/jobs/our-offices.md View 1 1 chunk +15 lines, -1 line 0 comments Download
M includes/jobs/overview.md View 1 2 1 chunk +16 lines, -4 lines 0 comments Download
A includes/jobs/recruitment-process.md View 1 1 chunk +23 lines, -0 lines 0 comments Download
M includes/jobs/why-work-with-us.md View 1 1 chunk +5 lines, -15 lines 0 comments Download
M pages/jobs/index.tmpl View 1 2 chunks +17 lines, -12 lines 0 comments Download
M static/css/styles.css View 1 chunk +2 lines, -1 line 0 comments Download

Messages

Total messages: 4
juliandoucette
2 years, 7 months ago (2017-07-12 19:32:50 UTC) #1
juliandoucette
One note below :) https://codereview.adblockplus.org/29487683/diff/29487692/static/css/styles.css File static/css/styles.css (right): https://codereview.adblockplus.org/29487683/diff/29487692/static/css/styles.css#newcode798 static/css/styles.css:798: .page-title > *, Note: I ...
2 years, 7 months ago (2017-07-12 20:54:54 UTC) #2
ire
Just a couple minor things, but otherwise LGTM https://codereview.adblockplus.org/29487683/diff/29487692/includes/jobs/employee-perks.md File includes/jobs/employee-perks.md (right): https://codereview.adblockplus.org/29487683/diff/29487692/includes/jobs/employee-perks.md#newcode4 includes/jobs/employee-perks.md:4: There's ...
2 years, 7 months ago (2017-07-13 20:00:41 UTC) #3
juliandoucette
2 years, 7 months ago (2017-07-14 10:46:04 UTC) #4
Thanks Ire - I'm going to go ahead and push this.

https://codereview.adblockplus.org/29487683/diff/29487692/includes/jobs/emplo...
File includes/jobs/employee-perks.md (right):

https://codereview.adblockplus.org/29487683/diff/29487692/includes/jobs/emplo...
includes/jobs/employee-perks.md:4: 
On 2017/07/13 20:00:40, ire wrote:
> There's a section titled "Work / life balance" that's missing from here.

Done.

Good eye!

https://codereview.adblockplus.org/29487683/diff/29487692/includes/jobs/overv...
File includes/jobs/overview.md (right):

https://codereview.adblockplus.org/29487683/diff/29487692/includes/jobs/overv...
includes/jobs/overview.md:2: <h2>Join us in changing the Internet!</h2>
On 2017/07/13 20:00:41, ire wrote:
> NIT: Is there a reason you're using HTML here instead of Markdown?

Done.

Semantics and personal preference. I had to wrap this H2 and p in a header to
communicate that the p is a subtitle. Of course, I could have added markdown="1"
to the header or just let the subtitle be a paragraph.

I probably should have done the latter.
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld 87257f5