> On Oct. 13, 2017, 2:48 p.m., Joshua Cohen wrote: > > When I implemented this originally I wanted it to be a separate URL > > (http://aurora/scheduler/role/env/job/completed) rather than a query > > parameter (?tab=completed), but due to something in Angular's router it > > wasn't really feasible at the time. Presumably this is easier to accomplish > > w/ React? > > > > I know [cool uris don't change](https://www.w3.org/Provider/Style/URI), but > > I think it'd be ok to drop the query support in favor of a full url (or, if > > we're really concerned about bookmarks have a handler that redirects the > > query param form to the url form)? > > > > What do you think?
Your suggestion is easy to accomplish with React Router and there is only ambiguity with instance ids, which are integers - so no real risk either. Aesthetically I like the cleaner REST-y URLs, but I also like the flexibility that the query params approach gives - e.g. later on your can combine multiple view parameters if you have multiple toggle views, etc.) which is why I leaned towards that. I'm pretty open to changing it though. Is there any other reason other than aesthetics you prefer the full url approach? - David ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/62958/#review187962 ----------------------------------------------------------- On Oct. 13, 2017, 12:10 a.m., David McLaughlin wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/62958/ > ----------------------------------------------------------- > > (Updated Oct. 13, 2017, 12:10 a.m.) > > > Review request for Aurora, Kai Huang, Reza Motamedi, and Santhosh Kumar > Shanmugham. > > > Repository: aurora > > > Description > ------- > > This maintains the existing UI functionality where clicking on tabs updates > the browser location, to create shareable URLs to tab views. > > > Diffs > ----- > > ui/package.json cde8d106346d9ac498cfee9b5291ebc637fc6a2a > ui/src/main/js/components/Tabs.js 43b1950b11b80dc3017730801992341bc527c39c > ui/src/main/js/components/__tests__/Tabs-test.js > e028c2dd86739ed9762aa1d0be5c609d5487e06e > ui/src/main/js/pages/Job.js fc400f7442a1f8a5f0ebfe366dfe40ef40e7108e > ui/src/main/js/pages/__tests__/Job-test.js > 4cc76b8731d71ceca87a5d1e259360b2af8feba0 > > > Diff: https://reviews.apache.org/r/62958/diff/1/ > > > Testing > ------- > > ./gradlew ui:lint > ./gradlew ui:test > > Unfortunately the testing here is only on one side (restoring the URL state). > I cannot simulate the change events from the unit tests because of > limitations with shallow rendering. When I figure out how to add integration > tests that work with React Router, I'll add coverage. > > > Thanks, > > David McLaughlin > >
