> On Sept. 27, 2017, 8:59 p.m., Santhosh Kumar Shanmugham wrote: > > ui/src/main/js/components/Pagination.js > > Lines 3 (patched) > > <https://reviews.apache.org/r/62607/diff/2/?file=1837566#file1837566line3> > > > > nit - `maxPages` and `numPages` are a little confusing. Can we use the > > terminology similar to Reactable - such as `itemsPerPage` and > > `pageButtonLimit` or similar?
itemsPerPage isn't represented here since the range is provide to Pages. I think conceptually the tricky thing here is to realize this is a cosmetic component for returning the actual page navigation and not doing any paging itself. So I've changed the name of the component to help reflect that. > On Sept. 27, 2017, 8:59 p.m., Santhosh Kumar Shanmugham wrote: > > ui/src/main/js/components/__tests__/Pagination-test.js > > Lines 155-158 (patched) > > <https://reviews.apache.org/r/62607/diff/3/?file=1837596#file1837596line155> > > > > The order will be «, 7, 8, 9 correct? enzyme isn't as good as my shallow renderer ;) I couldn't find a way to enforce ordering without having to match *all* properties (which is really difficult when you have props that are functions) so this is just verifying they are present. > On Sept. 27, 2017, 8:59 p.m., Santhosh Kumar Shanmugham wrote: > > ui/src/main/js/components/__tests__/Pagination-test.js > > Lines 167-173 (patched) > > <https://reviews.apache.org/r/62607/diff/3/?file=1837596#file1837596line167> > > > > nit - re-order? I actually did this intentionally to be very clear about the lack of order enforcement in the test assertion. But I can reorder it if needed. - David ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/62607/#review186472 ----------------------------------------------------------- On Sept. 27, 2017, 5:51 p.m., David McLaughlin wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/62607/ > ----------------------------------------------------------- > > (Updated Sept. 27, 2017, 5:51 p.m.) > > > Review request for Aurora, Kai Huang and Santhosh Kumar Shanmugham. > > > Repository: aurora > > > Description > ------- > > Replace Preact and custom testing with React + jest + Enzyme now that they > are MIT licensed. > > Side-effects: > > * The Reactable library is not compatible with React 16.0 (the MIT licensed > version we are forced to use) and attempts to flag this started as early as > June this year. But the author seems to have abandoned the library. So I had > to create my own Pagination class. I found myself repeatedly using Reactable > in places where I just wanted pagination (for lists of divs) anyway, so maybe > this is a good thing. > * Deleted custom (and questionable) shallow renderer. > * The download time will be reduced due to not having to download PhantomJS. > > > Diffs > ----- > > build.gradle 460500aaeab28dcfeb29ec602d057ea4bca12378 > ui/karma.conf.js d42cda7d025f8c5618039888f4fb49ff388c3af1 > ui/package.json d6802029e0aeb9f6692ec8cc065949ce17b2ca07 > ui/src/__mocks__/react.js PRE-CREATION > ui/src/main/js/components/Pagination.js PRE-CREATION > ui/src/main/js/components/RoleList.js > 32595601aae06730f537d37d95be2e746f779103 > ui/src/main/js/components/__tests__/Breadcrumb-test.js > 18af0fe6c360f375fb0fa1694304d821041d9e16 > ui/src/main/js/components/__tests__/Home-test.js > 8e6bc09050148ea9711dbfe6f52b83c8210262be > ui/src/main/js/components/__tests__/Pagination-test.js PRE-CREATION > ui/src/main/js/pages/__tests__/Home-test.js > 4f13f99ea24c0252b5f6cbb6980e0c96832d5120 > ui/src/main/js/utils/ShallowRender.js > 52e8bb259264e7dd47ee97cec35553acc147e818 > ui/src/main/js/utils/__tests__/ShallowRender-test.js > d5663a7c44de795a5aa0b28d4f373871ffcc3fd2 > ui/src/main/sass/components/_tables.scss > 58f176cffcdae34de7c776e31b3f0342efa3024f > ui/test-setup.js PRE-CREATION > ui/webpack.config.js e7cd672df62006bd5c5b60a6ba0903e16a767d13 > > > Diff: https://reviews.apache.org/r/62607/diff/3/ > > > Testing > ------- > > $ ./gradlew ui:lint > $ ./gradlew ui:test > > I also tested the UI in my Vagrant image. > > > Thanks, > > David McLaughlin > >
