Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2936423002:
config_service: add routing for config-set page. (Closed)
|
Created:
3 years, 6 months ago by ayanaadylova Modified:
3 years, 6 months ago CC:
chromium-reviews, infra-reviews+luci-py_chromium.org Target Ref:
refs/heads/master Project:
luci-py Visibility:
Public. |
Descriptionconfig_service: add routing for config-set page.
Add routing from front page to config-set page when
user clicks on config-set-card element.
BUG=730832
Review-Url: https://codereview.chromium.org/2936423002
Committed: https://github.com/luci/luci-py/commit/5fee7aeb5b070a05d81dfe3e3c8900504db5a0ec
Patch Set 1 #
Total comments: 18
Patch Set 2 : Add routing for config-set page. #Patch Set 3 : Change config-set name to reflect the full path. #
Total comments: 11
Patch Set 4 : Fix some styling issues. #Patch Set 5 : Remove UI handler and make static url for /newui. #Patch Set 6 : Remove UI handler. #Messages
Total messages: 32 (12 generated)
ayanaadylova@google.com changed reviewers: + hinoka@google.com, sergeyberezin@google.com
PTAL
hinoka@google.com changed reviewers: + seanmccullough@chromium.org
+sean: Can you take a look at this? It's a polymer 2.0 app skeleton, with routing done via app-location for binding the url address bar, and app-route for routing. URLs are #fragments to keep the single page cached. Nothing is vulcanized yet. https://codereview.chromium.org/2936423002/diff/1/appengine/config_service/ui... File appengine/config_service/ui/src/config-ui/config-ui.html (right): https://codereview.chromium.org/2936423002/diff/1/appengine/config_service/ui... appengine/config_service/ui/src/config-ui/config-ui.html:69: <config-set category="Service" config-set-name="{{serviceData.serviceName}}" route="{{serviceTail}}" ></config-set> nit: 80 char. The indentation style you used in Line 49 is fine. https://codereview.chromium.org/2936423002/diff/1/appengine/config_service/ui... appengine/config_service/ui/src/config-ui/config-ui.html:72: <config-set category="Project" config-set-name="{{projectData.projectName}}" route="{{projectTail}}"></config-set> nit: same here, 80 char https://codereview.chromium.org/2936423002/diff/1/appengine/config_service/ui... File appengine/config_service/ui/src/config-ui/front-page.html (right): https://codereview.chromium.org/2936423002/diff/1/appengine/config_service/ui... appengine/config_service/ui/src/config-ui/front-page.html:41: url="https://luci-config.appspot.com/_ah/api/config/v1/config-sets" In general, don't use full URLs. Use relative URLs from the root. That is, don't change this.
Can you upload this to a staging version somewhere and link to it in the comments or description of this CL? On 2017/06/15 18:55:57, Ryan Tseng wrote: > +sean: Can you take a look at this? It's a polymer 2.0 app skeleton, with > routing done via app-location for binding the url address bar, and app-route for > routing. URLs are #fragments to keep the single page cached. Nothing is > vulcanized yet. > > https://codereview.chromium.org/2936423002/diff/1/appengine/config_service/ui... > File appengine/config_service/ui/src/config-ui/config-ui.html (right): > > https://codereview.chromium.org/2936423002/diff/1/appengine/config_service/ui... > appengine/config_service/ui/src/config-ui/config-ui.html:69: <config-set > category="Service" config-set-name="{{serviceData.serviceName}}" > route="{{serviceTail}}" ></config-set> > nit: 80 char. The indentation style you used in Line 49 is fine. > > https://codereview.chromium.org/2936423002/diff/1/appengine/config_service/ui... > appengine/config_service/ui/src/config-ui/config-ui.html:72: <config-set > category="Project" config-set-name="{{projectData.projectName}}" > route="{{projectTail}}"></config-set> > nit: same here, 80 char > > https://codereview.chromium.org/2936423002/diff/1/appengine/config_service/ui... > File appengine/config_service/ui/src/config-ui/front-page.html (right): > > https://codereview.chromium.org/2936423002/diff/1/appengine/config_service/ui... > appengine/config_service/ui/src/config-ui/front-page.html:41: > url="https://luci-config.appspot.com/_ah/api/config/v1/config-sets" > In general, don't use full URLs. Use relative URLs from the root. That is, > don't change this.
seanmccullough@google.com changed reviewers: + seanmccullough@google.com
https://codereview.chromium.org/2936423002/diff/1/appengine/config_service/ui... File appengine/config_service/ui/src/config-ui/config-ui.html (right): https://codereview.chromium.org/2936423002/diff/1/appengine/config_service/ui... appengine/config_service/ui/src/config-ui/config-ui.html:43: <div class="title" main-title>Configuration Service (not fully implemented)</div> 80char https://codereview.chromium.org/2936423002/diff/1/appengine/config_service/ui... appengine/config_service/ui/src/config-ui/config-ui.html:48: <app-location route="{{route}}" use-hash-as-path></app-location> Every browser we care about supports html5 navigation API. What breaks when you take use-hash-as-path out and go with the default? Does the server-side routing not handle it well? https://codereview.chromium.org/2936423002/diff/1/appengine/config_service/ui... appengine/config_service/ui/src/config-ui/config-ui.html:65: <div hidden$="{{!frontPageActive}}"> [[ ]] instead of {{ }}. You don't need two-way binding for this property. Same with the other *PageActive attributes below. https://codereview.chromium.org/2936423002/diff/1/appengine/config_service/ui... appengine/config_service/ui/src/config-ui/config-ui.html:78: Polymer({ IIUC you can use native ES6 classes instead of the Polymer({}) factory with polymer 2.0. This should work too though. https://codereview.chromium.org/2936423002/diff/1/appengine/config_service/ui... File appengine/config_service/ui/src/config-ui/front-page.html (right): https://codereview.chromium.org/2936423002/diff/1/appengine/config_service/ui... appengine/config_service/ui/src/config-ui/front-page.html:60: <div on-click="_handleClick"> do you need this on-click handler on the wrapper div? Why can't config-set-card handle its own clicks (also, I prefer to use on-tap instead of on-click, but it shouldn't matter unless this goes mobile).
https://codereview.chromium.org/2936423002/diff/1/appengine/config_service/ui... File appengine/config_service/ui/src/config-ui/config-ui.html (right): https://codereview.chromium.org/2936423002/diff/1/appengine/config_service/ui... appengine/config_service/ui/src/config-ui/config-ui.html:48: <app-location route="{{route}}" use-hash-as-path></app-location> On 2017/06/15 19:50:00, seanmccullough1 wrote: > Every browser we care about supports html5 navigation API. > > What breaks when you take use-hash-as-path out and go with the default? Does the > server-side routing not handle it well? We're doing it this way because for example "/" is cached differently than "/projects/blah". So even with the page cached, if the browser hits a different URL, it'll try to fetch the whole app again. I think this can be fixed with service workers, but that's more complexity than we want to do initially for now.
Link to the staging version: https://2901-2279363-tainted-ayanaadylova-dot-luci-config-dev.appspot.com/newui PTAL https://codereview.chromium.org/2936423002/diff/1/appengine/config_service/ui... File appengine/config_service/ui/src/config-ui/config-ui.html (right): https://codereview.chromium.org/2936423002/diff/1/appengine/config_service/ui... appengine/config_service/ui/src/config-ui/config-ui.html:43: <div class="title" main-title>Configuration Service (not fully implemented)</div> On 2017/06/15 19:50:00, seanmccullough1 wrote: > 80char Done. https://codereview.chromium.org/2936423002/diff/1/appengine/config_service/ui... appengine/config_service/ui/src/config-ui/config-ui.html:48: <app-location route="{{route}}" use-hash-as-path></app-location> On 2017/06/15 20:01:54, Ryan Tseng wrote: > On 2017/06/15 19:50:00, seanmccullough1 wrote: > > Every browser we care about supports html5 navigation API. > > > > What breaks when you take use-hash-as-path out and go with the default? Does > the > > server-side routing not handle it well? > > We're doing it this way because for example "/" is cached differently than > "/projects/blah". So even with the page cached, if the browser hits a different > URL, it'll try to fetch the whole app again. I think this can be fixed with > service workers, but that's more complexity than we want to do initially for > now. Done. https://codereview.chromium.org/2936423002/diff/1/appengine/config_service/ui... appengine/config_service/ui/src/config-ui/config-ui.html:48: <app-location route="{{route}}" use-hash-as-path></app-location> On 2017/06/15 19:50:00, seanmccullough1 wrote: > Every browser we care about supports html5 navigation API. > > What breaks when you take use-hash-as-path out and go with the default? Does the > server-side routing not handle it well? Done. https://codereview.chromium.org/2936423002/diff/1/appengine/config_service/ui... appengine/config_service/ui/src/config-ui/config-ui.html:65: <div hidden$="{{!frontPageActive}}"> On 2017/06/15 19:50:01, seanmccullough1 wrote: > [[ ]] instead of {{ }}. You don't need two-way binding for this property. Same > with the other *PageActive attributes below. Done. https://codereview.chromium.org/2936423002/diff/1/appengine/config_service/ui... appengine/config_service/ui/src/config-ui/config-ui.html:69: <config-set category="Service" config-set-name="{{serviceData.serviceName}}" route="{{serviceTail}}" ></config-set> On 2017/06/15 18:55:56, Ryan Tseng wrote: > nit: 80 char. The indentation style you used in Line 49 is fine. Done. https://codereview.chromium.org/2936423002/diff/1/appengine/config_service/ui... appengine/config_service/ui/src/config-ui/config-ui.html:72: <config-set category="Project" config-set-name="{{projectData.projectName}}" route="{{projectTail}}"></config-set> On 2017/06/15 18:55:56, Ryan Tseng wrote: > nit: same here, 80 char Done. https://codereview.chromium.org/2936423002/diff/1/appengine/config_service/ui... appengine/config_service/ui/src/config-ui/config-ui.html:78: Polymer({ On 2017/06/15 19:50:01, seanmccullough1 wrote: > IIUC you can use native ES6 classes instead of the Polymer({}) factory with > polymer 2.0. This should work too though. Acknowledged. https://codereview.chromium.org/2936423002/diff/1/appengine/config_service/ui... File appengine/config_service/ui/src/config-ui/front-page.html (right): https://codereview.chromium.org/2936423002/diff/1/appengine/config_service/ui... appengine/config_service/ui/src/config-ui/front-page.html:41: url="https://luci-config.appspot.com/_ah/api/config/v1/config-sets" On 2017/06/15 18:55:56, Ryan Tseng wrote: > In general, don't use full URLs. Use relative URLs from the root. That is, > don't change this. Done. https://codereview.chromium.org/2936423002/diff/1/appengine/config_service/ui... appengine/config_service/ui/src/config-ui/front-page.html:60: <div on-click="_handleClick"> On 2017/06/15 19:50:01, seanmccullough1 wrote: > do you need this on-click handler on the wrapper div? Why can't config-set-card > handle its own clicks (also, I prefer to use on-tap instead of on-click, but it > shouldn't matter unless this goes mobile). Done.
On 2017/06/15 21:26:01, ayanaadylova wrote: > Link to the staging version: > https://2901-2279363-tainted-ayanaadylova-dot-luci-config-dev.appspot.com/newui > > PTAL > > https://codereview.chromium.org/2936423002/diff/1/appengine/config_service/ui... > File appengine/config_service/ui/src/config-ui/config-ui.html (right): > > https://codereview.chromium.org/2936423002/diff/1/appengine/config_service/ui... > appengine/config_service/ui/src/config-ui/config-ui.html:43: <div class="title" > main-title>Configuration Service (not fully implemented)</div> > On 2017/06/15 19:50:00, seanmccullough1 wrote: > > 80char > > Done. > > https://codereview.chromium.org/2936423002/diff/1/appengine/config_service/ui... > appengine/config_service/ui/src/config-ui/config-ui.html:48: <app-location > route="{{route}}" use-hash-as-path></app-location> > On 2017/06/15 20:01:54, Ryan Tseng wrote: > > On 2017/06/15 19:50:00, seanmccullough1 wrote: > > > Every browser we care about supports html5 navigation API. > > > > > > What breaks when you take use-hash-as-path out and go with the default? Does > > the > > > server-side routing not handle it well? > > > > We're doing it this way because for example "/" is cached differently than > > "/projects/blah". So even with the page cached, if the browser hits a > different > > URL, it'll try to fetch the whole app again. I think this can be fixed with > > service workers, but that's more complexity than we want to do initially for > > now. > > Done. > > https://codereview.chromium.org/2936423002/diff/1/appengine/config_service/ui... > appengine/config_service/ui/src/config-ui/config-ui.html:48: <app-location > route="{{route}}" use-hash-as-path></app-location> > On 2017/06/15 19:50:00, seanmccullough1 wrote: > > Every browser we care about supports html5 navigation API. > > > > What breaks when you take use-hash-as-path out and go with the default? Does > the > > server-side routing not handle it well? > > Done. > > https://codereview.chromium.org/2936423002/diff/1/appengine/config_service/ui... > appengine/config_service/ui/src/config-ui/config-ui.html:65: <div > hidden$="{{!frontPageActive}}"> > On 2017/06/15 19:50:01, seanmccullough1 wrote: > > [[ ]] instead of {{ }}. You don't need two-way binding for this property. Same > > with the other *PageActive attributes below. > > Done. > > https://codereview.chromium.org/2936423002/diff/1/appengine/config_service/ui... > appengine/config_service/ui/src/config-ui/config-ui.html:69: <config-set > category="Service" config-set-name="{{serviceData.serviceName}}" > route="{{serviceTail}}" ></config-set> > On 2017/06/15 18:55:56, Ryan Tseng wrote: > > nit: 80 char. The indentation style you used in Line 49 is fine. > > Done. > > https://codereview.chromium.org/2936423002/diff/1/appengine/config_service/ui... > appengine/config_service/ui/src/config-ui/config-ui.html:72: <config-set > category="Project" config-set-name="{{projectData.projectName}}" > route="{{projectTail}}"></config-set> > On 2017/06/15 18:55:56, Ryan Tseng wrote: > > nit: same here, 80 char > > Done. > > https://codereview.chromium.org/2936423002/diff/1/appengine/config_service/ui... > appengine/config_service/ui/src/config-ui/config-ui.html:78: Polymer({ > On 2017/06/15 19:50:01, seanmccullough1 wrote: > > IIUC you can use native ES6 classes instead of the Polymer({}) factory with > > polymer 2.0. This should work too though. > > Acknowledged. > > https://codereview.chromium.org/2936423002/diff/1/appengine/config_service/ui... > File appengine/config_service/ui/src/config-ui/front-page.html (right): > > https://codereview.chromium.org/2936423002/diff/1/appengine/config_service/ui... > appengine/config_service/ui/src/config-ui/front-page.html:41: > url="https://luci-config.appspot.com/_ah/api/config/v1/config-sets" > On 2017/06/15 18:55:56, Ryan Tseng wrote: > > In general, don't use full URLs. Use relative URLs from the root. That is, > > don't change this. > > Done. > > https://codereview.chromium.org/2936423002/diff/1/appengine/config_service/ui... > appengine/config_service/ui/src/config-ui/front-page.html:60: <div > on-click="_handleClick"> > On 2017/06/15 19:50:01, seanmccullough1 wrote: > > do you need this on-click handler on the wrapper div? Why can't > config-set-card > > handle its own clicks (also, I prefer to use on-tap instead of on-click, but > it > > shouldn't matter unless this goes mobile). > > Done. I changed the name of the config-set to reflect the full path. PTAL
Sorry I didn't review it yesterday - catching up today. Overall it looks pretty good, and I tried it on the staging server, works nicely! Some more comments, and it looks like config-set-card.html file is missing - please 'git add' it before uploading. Thanks! https://codereview.chromium.org/2936423002/diff/40001/appengine/config_servic... File appengine/config_service/handlers.py (right): https://codereview.chromium.org/2936423002/diff/40001/appengine/config_servic... appengine/config_service/handlers.py:18: nit: 2 empty lines between top-level statements, here and below. https://codereview.chromium.org/2936423002/diff/40001/appengine/config_servic... appengine/config_service/handlers.py:23: nit: 2 empty lines https://codereview.chromium.org/2936423002/diff/40001/appengine/config_servic... appengine/config_service/handlers.py:40: are ready to deploy this on the main url (luci-config.appspot.com). nit: even if temporary, I'd remove the reference to the specific instance, e.g.: s/luci-config.appspot.com/<domain>/ https://codereview.chromium.org/2936423002/diff/40001/appengine/config_servic... appengine/config_service/handlers.py:44: template = JINJA_ENVIRONMENT.get_template('ui/index.html') ui/index.html doesn't seem to use any jinja template features, so IIUC this ends up just serving the file straight up. Is there any advantage of using jinja as opposed to a static URL configured in app.yaml? https://codereview.chromium.org/2936423002/diff/40001/appengine/config_servic... File appengine/config_service/ui/src/config-ui/front-page.html (right): https://codereview.chromium.org/2936423002/diff/40001/appengine/config_servic... appengine/config_service/ui/src/config-ui/front-page.html:41: url="https://luci-config.appspot.com/_ah/api/config/v1/config-sets" IMHO, we should keep URLs relative from the start. It's OK to debug locally with a specific instance URL, but when committing code, I'd rather not leave it here, since it may slip into the final production version.
PTAL https://codereview.chromium.org/2936423002/diff/40001/appengine/config_servic... File appengine/config_service/handlers.py (right): https://codereview.chromium.org/2936423002/diff/40001/appengine/config_servic... appengine/config_service/handlers.py:18: On 2017/06/16 20:25:23, Sergey Berezin wrote: > nit: 2 empty lines between top-level statements, here and below. Done. https://codereview.chromium.org/2936423002/diff/40001/appengine/config_servic... appengine/config_service/handlers.py:23: On 2017/06/16 20:25:23, Sergey Berezin wrote: > nit: 2 empty lines Done. https://codereview.chromium.org/2936423002/diff/40001/appengine/config_servic... appengine/config_service/handlers.py:40: are ready to deploy this on the main url (luci-config.appspot.com). On 2017/06/16 20:25:23, Sergey Berezin wrote: > nit: even if temporary, I'd remove the reference to the specific instance, e.g.: > s/luci-config.appspot.com/<domain>/ Done. https://codereview.chromium.org/2936423002/diff/40001/appengine/config_servic... appengine/config_service/handlers.py:44: template = JINJA_ENVIRONMENT.get_template('ui/index.html') On 2017/06/16 20:25:23, Sergey Berezin wrote: > ui/index.html doesn't seem to use any jinja template features, so IIUC this ends > up just serving the file straight up. Is there any advantage of using jinja as > opposed to a static URL configured in app.yaml? I am using jinja to render a template because it is sort of similar to how Django, which I have used before, does it. I also have seen people using Flask. However, I am not sure if there is a way to do it without a template environment. https://codereview.chromium.org/2936423002/diff/40001/appengine/config_servic... File appengine/config_service/ui/src/config-ui/front-page.html (right): https://codereview.chromium.org/2936423002/diff/40001/appengine/config_servic... appengine/config_service/ui/src/config-ui/front-page.html:41: url="https://luci-config.appspot.com/_ah/api/config/v1/config-sets" On 2017/06/16 20:25:23, Sergey Berezin wrote: > IMHO, we should keep URLs relative from the start. It's OK to debug locally with > a specific instance URL, but when committing code, I'd rather not leave it here, > since it may slip into the final production version. Done.
hinoka@google.com changed reviewers: + hinoka@google.com
https://codereview.chromium.org/2936423002/diff/40001/appengine/config_servic... File appengine/config_service/handlers.py (right): https://codereview.chromium.org/2936423002/diff/40001/appengine/config_servic... appengine/config_service/handlers.py:44: template = JINJA_ENVIRONMENT.get_template('ui/index.html') On 2017/06/16 21:34:46, ayanaadylova wrote: > On 2017/06/16 20:25:23, Sergey Berezin wrote: > > ui/index.html doesn't seem to use any jinja template features, so IIUC this > ends > > up just serving the file straight up. Is there any advantage of using jinja as > > opposed to a static URL configured in app.yaml? > > I am using jinja to render a template because it is sort of similar to how > Django, which I have used before, does it. I also have seen people using Flask. > However, I am not sure if there is a way to do it without a template > environment. Swarming uses jinja to inject the Google Analytics ID. Other than that, static websites probably would be a better idea: https://cloud.google.com/appengine/docs/standard/python/getting-started/hosti...
On 2017/06/16 22:08:01, Ryan Tseng wrote: > https://codereview.chromium.org/2936423002/diff/40001/appengine/config_servic... > File appengine/config_service/handlers.py (right): > > https://codereview.chromium.org/2936423002/diff/40001/appengine/config_servic... > appengine/config_service/handlers.py:44: template = > JINJA_ENVIRONMENT.get_template('ui/index.html') > On 2017/06/16 21:34:46, ayanaadylova wrote: > > On 2017/06/16 20:25:23, Sergey Berezin wrote: > > > ui/index.html doesn't seem to use any jinja template features, so IIUC this > > ends > > > up just serving the file straight up. Is there any advantage of using jinja > as > > > opposed to a static URL configured in app.yaml? > > > > I am using jinja to render a template because it is sort of similar to how > > Django, which I have used before, does it. I also have seen people using > Flask. > > However, I am not sure if there is a way to do it without a template > > environment. > > Swarming uses jinja to inject the Google Analytics ID. Other than that, static > websites probably would be a better idea: > https://cloud.google.com/appengine/docs/standard/python/getting-started/hosti... Changed that and moved index.html to static directory.
I think you need to add the moved files back in and re-upload, I don't see them in the review.
sergeyberezin@google.com changed reviewers: + sergeyberezin@google.com
LGTM, provided you tested it on the staging server and it works. Thanks! I didn't notice any missing files, but just in case verify it with 'git status'.
On 2017/06/19 19:13:02, Sergey Berezin (google) wrote: > LGTM, provided you tested it on the staging server and it works. Thanks! > > I didn't notice any missing files, but just in case verify it with 'git status'. I am sorry, forgot to reply and tell that I added index.html in the patch set 6.
sergeyberezin@chromium.org changed reviewers: - sergeyberezin@google.com
The CQ bit was checked by ayanaadylova@google.com
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: Luci-py Presubmit on luci.infra.try (JOB_FAILED, https://luci-milo.appspot.com/swarming/task/36db64e196d73910)
LGTM from the correct account. Sorry :-)
The CQ bit was checked by ayanaadylova@google.com
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 100001, "attempt_start_ts": 1497914399451950,
"parent_rev": "cef4d8147899c356f5d2d2e7dd2ccc0bcabab088", "commit_rev":
"5fee7aeb5b070a05d81dfe3e3c8900504db5a0ec"}
Message was sent while issue was closed.
Description was changed from ========== config_service: add routing for config-set page. Add routing from front page to config-set page when user clicks on config-set-card element. BUG=730832 ========== to ========== config_service: add routing for config-set page. Add routing from front page to config-set page when user clicks on config-set-card element. BUG=730832 Review-Url: https://codereview.chromium.org/2936423002 Committed: https://github.com/luci/luci-py/commit/5fee7aeb5b070a05d81dfe3e3c8900504db5a0ec ==========
Message was sent while issue was closed.
Committed patchset #6 (id:100001) as https://github.com/luci/luci-py/commit/5fee7aeb5b070a05d81dfe3e3c8900504db5a0ec |
