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

Issue 29979564: Issue 5452 - Improve CMS API for local link generation (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
10 months, 1 week ago by rhowell
Modified:
10 months ago
Reviewers:
Vasily Kuznetsov
CC:
juliandoucette
Base URL:
https://hg.adblockplus.org/cms/
Visibility:
Public.

Description

Issue 5452 - Improve CMS API for local link generation

Patch Set 1 #

Total comments: 4

Patch Set 2 : Address comments on PS1 #

Total comments: 4

Patch Set 3 : Address comments on PS2 #

Total comments: 2

Patch Set 4 : Fix spacing in `global` #

Unified diffs Side-by-side diffs Delta from patch set Stats (+45 lines, -5 lines) Patch
M cms/converters.py View 1 2 chunks +12 lines, -0 lines 0 comments Download
M tests/expected_output/common/en/global View 1 2 3 1 chunk +6 lines, -1 line 0 comments Download
M tests/expected_output/dynamic/en/get_page_url View 1 2 1 chunk +4 lines, -0 lines 0 comments Download
A tests/expected_output/static/de/get_page_url View 1 2 1 chunk +9 lines, -0 lines 0 comments Download
M tests/expected_output/static/en/get_page_url View 1 2 1 chunk +4 lines, -0 lines 0 comments Download
M tests/test_site/pages/get_page_url.tmpl View 1 2 1 chunk +4 lines, -0 lines 0 comments Download
M tests/test_site/pages/global.tmpl View 1 2 3 1 chunk +6 lines, -4 lines 0 comments Download

Messages

Total messages: 8
rhowell
10 months, 1 week ago (2019-01-11 23:46:15 UTC) #1
Vasily Kuznetsov
Hi Rosie, Looks good. I just have one suggestion on the code below. Also, we ...
10 months ago (2019-01-14 13:29:27 UTC) #2
rhowell
Hi Vasily! I added a test for page_has_locale() in the globals.tmpl file. It seemed to ...
10 months ago (2019-01-14 14:56:47 UTC) #3
Vasily Kuznetsov
Hi Rosie, A couple more suggestions: https://codereview.adblockplus.org/29979564/diff/29981555/tests/expected_output/common/en/global File tests/expected_output/common/en/global (right): https://codereview.adblockplus.org/29979564/diff/29981555/tests/expected_output/common/en/global#newcode1 tests/expected_output/common/en/global:1: METHOD TEST PASSEDFalseTrueexistsTrueFalse ...
10 months ago (2019-01-14 16:14:06 UTC) #4
rhowell
Hi Vasily, Thanks for the good suggestions! Let me know if you see anything else? ...
10 months ago (2019-01-14 16:59:33 UTC) #5
Vasily Kuznetsov
LGTM I have one small suggestion, but it's just pure cosmetics. https://codereview.adblockplus.org/29979564/diff/29981585/tests/expected_output/common/en/global File tests/expected_output/common/en/global (right): ...
10 months ago (2019-01-16 10:32:14 UTC) #6
rhowell
Yeah, definitely looks better this way. Thanks, Vasily! :) https://codereview.adblockplus.org/29979564/diff/29981585/tests/expected_output/common/en/global File tests/expected_output/common/en/global (right): https://codereview.adblockplus.org/29979564/diff/29981585/tests/expected_output/common/en/global#newcode1 tests/expected_output/common/en/global:1: ...
10 months ago (2019-01-17 03:15:22 UTC) #7
Vasily Kuznetsov
10 months ago (2019-01-17 10:06:03 UTC) #8
On 2019/01/17 03:15:22, rhowell wrote:
> Yeah, definitely looks better this way. Thanks, Vasily!  :)
> 
>
https://codereview.adblockplus.org/29979564/diff/29981585/tests/expected_outp...
> File tests/expected_output/common/en/global (right):
> 
>
https://codereview.adblockplus.org/29979564/diff/29981585/tests/expected_outp...
> tests/expected_output/common/en/global:1: site_globals('test') =METHOD TEST
> PASSED
> On 2019/01/16 10:32:13, Vasily Kuznetsov wrote:
> > Perhaps we can now remove the "-" from "{{- ... }}" in the template so that
we
> > have more balanced whitespace here.
> 
> Done.

Even more LGTM!
Sign in to reply to this message.

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