----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/62135/#review185082 -----------------------------------------------------------
Ship it! lgtm overall. One question: do you think it's worthwhile to extract the shallow render to its own npm module that we depend on? Maybe that would make it possible/easier to have it upstreamed to preact? No idea if there are any challenges to that from an OSS perspective (i.e. we're ok to contribute to Aurora, but creating a new OSS project/publishing to npm/etc. may need an internal OSS review at Twitter?). ui/src/main/js/components/Breadcrumb.js Lines 12 (patched) <https://reviews.apache.org/r/62135/#comment261299> This is a change for the current behavior of the breadcrumbs in that currently the last crumb is not clickable. Is this the intent? I think the current behavior is preferable, but don't feel super strongly about it. Here's how I handled it in the original react prototype (which I remember feeling was kind of hacky truth be told): https://github.com/jcohen/aurora-react/blob/master/src/components/Breadcrumbs.js ui/src/main/js/components/Home.js Line 4 (original), 3-5 (patched) <https://reviews.apache.org/r/62135/#comment261300> Just curious, why the move away from the ES6 style? - Joshua Cohen On Sept. 8, 2017, 11:40 p.m., David McLaughlin wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/62135/ > ----------------------------------------------------------- > > (Updated Sept. 8, 2017, 11:40 p.m.) > > > Review request for Aurora, Joshua Cohen, Kai Huang, and Santhosh Kumar > Shanmugham. > > > Repository: aurora > > > Description > ------- > > Implementation of the home page in PreactJS. This was delayed heavily by the > lack of a shallow renderer > (https://facebook.github.io/react/docs/shallow-renderer.html) for testing. It > was too painful to test without it (e.g. Links when fully rendered must be > rendered within a Router context), so I hand-rolled one based on some > community attempts. The key motivation is just to allow us to decouple markup > from component logic, and also to avoid having to test child components > within component heirarchies. IMO the end result is very clean and easy to > read (and write!). > > If Preact gets a shallow renderer in the future, we can swap the one included > here out. > > > Diffs > ----- > > src/main/resources/scheduler/assets/images/aurora_logo_white.png > PRE-CREATION > ui/.eslintrc 355b6a8a5f5d25dd4c71dc546dc7d4acfffdd506 > ui/package.json f712518b27477bccb03f49c86eac3ee5f769fc88 > ui/src/main/js/client/scheduler-client.js PRE-CREATION > ui/src/main/js/components/Breadcrumb.js PRE-CREATION > ui/src/main/js/components/Home.js 91d60b387cf3b1fb268e73b7b50922a83898c31f > ui/src/main/js/components/Icon.js PRE-CREATION > ui/src/main/js/components/Loading.js PRE-CREATION > ui/src/main/js/components/Navigation.js PRE-CREATION > ui/src/main/js/components/RoleList.js PRE-CREATION > ui/src/main/js/components/__tests__/Breadcrumb-test.js PRE-CREATION > ui/src/main/js/components/__tests__/Home-test.js > 2a80958d9303d1d3b9ae8f95013c66cb39f6bac3 > ui/src/main/js/index.js 2f7467b41ac3373a90c5453a7534d384a585b464 > ui/src/main/js/pages/Home.js PRE-CREATION > ui/src/main/js/pages/__tests__/Home-test.js PRE-CREATION > ui/src/main/js/utils/ShallowRender.js PRE-CREATION > ui/src/main/js/utils/__tests__/ShallowRender-test.js PRE-CREATION > ui/src/main/sass/app.scss PRE-CREATION > ui/src/main/sass/components/_base.scss PRE-CREATION > ui/src/main/sass/components/_breadcrumb.scss PRE-CREATION > ui/src/main/sass/components/_home-page.scss PRE-CREATION > ui/src/main/sass/components/_layout.scss PRE-CREATION > ui/src/main/sass/components/_navigation.scss PRE-CREATION > ui/src/main/sass/components/_tables.scss PRE-CREATION > ui/src/main/sass/modules/_all.scss PRE-CREATION > ui/src/main/sass/modules/_colors.scss PRE-CREATION > ui/src/main/sass/modules/_typography.scss PRE-CREATION > > > Diff: https://reviews.apache.org/r/62135/diff/1/ > > > Testing > ------- > > ./gradlew ui:test > > Running in vagrant (screenshots attached). > > > File Attachments > ---------------- > > preview > > https://reviews.apache.org/media/uploaded/files/2017/09/08/011f99fb-a14e-4547-b90d-a5a1909c737c__Screen_Shot_2017-09-08_at_3.58.48_PM.png > > > Thanks, > > David McLaughlin > >
