Mailing-List: contact reviews-help@aurora.apache.org; run by ezmlm
Precedence: bulk
Reply-To: reviews@aurora.apache.org
Content-Type: multipart/alternative;
 boundary="===============4129439540033882580=="
MIME-Version: 1.0
Subject: Re: Review Request 62607: Replace Preact and custom testing with
 React + Enzyme
From: Santhosh Kumar Shanmugham <santhoshkumar.s@gmail.com>
To: Santhosh Kumar Shanmugham <santhoshkumar.s@gmail.com>,
 Kai Huang <texasred2013@hotmail.com>
Cc: David McLaughlin <david@dmclaughlin.com>,
 Aurora <reviews@aurora.apache.org>
Date: Wed, 27 Sep 2017 20:59:21 -0000
Message-ID: <20170927205921.32396.34327@reviews-vm2.apache.org>
Auto-Submitted: auto-generated
Sender: Santhosh Kumar Shanmugham <noreply@reviews.apache.org>
References: <20170927175108.32359.25398@reviews-vm2.apache.org>
In-Reply-To: <20170927175108.32359.25398@reviews-vm2.apache.org>
Reply-To: Santhosh Kumar Shanmugham <santhoshkumar.s@gmail.com>
archived-at: Wed, 27 Sep 2017 20:59:30 -0000

--===============4129439540033882580==
MIME-Version: 1.0
Content-Type: text/plain; charset="utf-8"
Content-Transfer-Encoding: 7bit


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62607/#review186472
-----------------------------------------------------------


Ship it!


LGTM. Minor comments about readability.


ui/src/main/js/components/Pagination.js
Lines 3 (patched)
<https://reviews.apache.org/r/62607/#comment263027>

    nit - `maxPages` and `numPages` are a little confusing. Can we use the terminology similar to Reactable - such as `itemsPerPage` and `pageButtonLimit` or similar?


ui/src/main/js/components/Pagination.js
Lines 3 (patched)
<https://reviews.apache.org/r/62607/#comment263044>

    nit - maxPages and numPages are a little confusing. Can we use the terminology similar to Reactable - such as itemsPerPage and pageButtonLimit or similar?


ui/src/main/js/components/__tests__/Pagination-test.js
Lines 155-158 (patched)
<https://reviews.apache.org/r/62607/#comment263042>

    The order will be &laquo;, 7, 8, 9 correct?


ui/src/main/js/components/__tests__/Pagination-test.js
Lines 167-173 (patched)
<https://reviews.apache.org/r/62607/#comment263045>

    nit - re-order?


- Santhosh Kumar Shanmugham


On Sept. 27, 2017, 10:51 a.m., David McLaughlin wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62607/
> -----------------------------------------------------------
> 
> (Updated Sept. 27, 2017, 10:51 a.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
> 
>


--===============4129439540033882580==--