Mailing-List: contact reviews-help@aurora.apache.org; run by ezmlm
Precedence: bulk
Reply-To: reviews@aurora.apache.org
Content-Type: multipart/alternative;
 boundary="===============4678234738266054219=="
MIME-Version: 1.0
Subject: Re: Review Request 62908: Implement Job page in React
From: David McLaughlin <david@dmclaughlin.com>
To: Santhosh Kumar Shanmugham <santhoshkumar.s@gmail.com>,
 Kai Huang <texasred2013@hotmail.com>
Cc: Aurora ReviewBot <wfarner@apache.org>, Aurora <reviews@aurora.apache.org>,
 David McLaughlin <david@dmclaughlin.com>
Date: Thu, 12 Oct 2017 02:46:58 -0000
Message-ID: <20171012024658.32091.3523@reviews-vm2.apache.org>
Auto-Submitted: auto-generated
Sender: David McLaughlin <noreply@reviews.apache.org>
References: <20171012013139.32091.66437@reviews-vm2.apache.org>
In-Reply-To: <20171012013139.32091.66437@reviews-vm2.apache.org>
Reply-To: David McLaughlin <david@dmclaughlin.com>
archived-at: Thu, 12 Oct 2017 02:47:05 -0000

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


> On Oct. 12, 2017, 1:31 a.m., Kai Huang wrote:
> > ui/src/main/js/components/ConfigDiff.js
> > Lines 19 (patched)
> > <https://reviews.apache.org/r/62908/diff/1/?file=1852968#file1852968line19>
> >
> >     1. Why use `diff-before` here if the css style has applied in the wrapper of picker(divNavigation below)?
> >     
> >     2. Is this an unimplemented css style?
> >     
> >     Perhaps we should add a legend alongside the instanceGroup picker, so that it's more intuitive to figure out the color theory.

For (1) and (2), I use the CSS selector in the unit test. The duplicate style is an artifact of refactor to reuse code. I'll remove it. 

I think red/green for diffing is a pretty established convention and doesn't need a legend.


> On Oct. 12, 2017, 1:31 a.m., Kai Huang wrote:
> > ui/src/main/js/components/JobConfig.js
> > Lines 16 (patched)
> > <https://reviews.apache.org/r/62908/diff/1/?file=1852969#file1852969line16>
> >
> >     UX:
> >     When there are 3 configs on the page, there is a lot of empty space on the right. Can we fill each row by 3 columns?

It's supposed to be 3 columns, but in that screenshot the data in the table (the longest metadata value) is stretching the width. There is no character breaks. So I think this is the best we can do without making the font tiny.


> On Oct. 12, 2017, 1:31 a.m., Kai Huang wrote:
> > ui/src/main/js/components/TaskConfigSummary.js
> > Lines 55 (patched)
> > <https://reviews.apache.org/r/62908/diff/1/?file=1852972#file1852972line55>
> >
> >     remove the `?

Done.


> On Oct. 12, 2017, 1:31 a.m., Kai Huang wrote:
> > ui/src/main/js/components/__tests__/ConfigDiff-test.js
> > Lines 44 (patched)
> > <https://reviews.apache.org/r/62908/diff/1/?file=1852977#file1852977line44>
> >
> >     add a space: dropdownwhen

Done.


> On Oct. 12, 2017, 1:31 a.m., Kai Huang wrote:
> > ui/src/main/js/pages/Job.js
> > Lines 3 (patched)
> > <https://reviews.apache.org/r/62908/diff/1/?file=1852982#file1852982line3>
> >
> >     It appers the link in the navigation Breadcrumb still points to the old scheduler ui. We should fixing the links since it's going to production.

Fixed.


- David


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


On Oct. 12, 2017, 2:46 a.m., David McLaughlin wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62908/
> -----------------------------------------------------------
> 
> (Updated Oct. 12, 2017, 2:46 a.m.)
> 
> 
> Review request for Aurora, Kai Huang and Santhosh Kumar Shanmugham.
> 
> 
> Repository: aurora
> 
> 
> Description
> -------
> 
> Job page implemented in React. This is the last of the big commits before a bug bash and other small nits than can (hopefully) be collaborated on. 
> 
> Added deep configuration diff widget (for jobs with multiple configs) but otherwise very similar to the existing page.
> 
> 
> Diffs
> -----
> 
>   ui/package.json f4532dfcb7c7b65445190a8039eca5d89b4dcd56 
>   ui/src/main/js/components/Breadcrumb.js 76c62709cf3ebf76e74e08573bc8b245a45d5fe9 
>   ui/src/main/js/components/ConfigDiff.js PRE-CREATION 
>   ui/src/main/js/components/JobConfig.js PRE-CREATION 
>   ui/src/main/js/components/Pagination.js 0aac09d7360eee1cf971b35d1874afe4fa95b0f7 
>   ui/src/main/js/components/Tabs.js PRE-CREATION 
>   ui/src/main/js/components/TaskConfigSummary.js PRE-CREATION 
>   ui/src/main/js/components/TaskList.js PRE-CREATION 
>   ui/src/main/js/components/TaskStateMachine.js PRE-CREATION 
>   ui/src/main/js/components/UpdateList.js 3f57669140b04b29e12003d68ded387825e83f85 
>   ui/src/main/js/components/UpdatePreview.js PRE-CREATION 
>   ui/src/main/js/components/__tests__/Breadcrumb-test.js 47f7afb29b9192e214040097dbfee2ff7f7e29ed 
>   ui/src/main/js/components/__tests__/ConfigDiff-test.js PRE-CREATION 
>   ui/src/main/js/components/__tests__/JobConfig-test.js PRE-CREATION 
>   ui/src/main/js/components/__tests__/Tabs-test.js PRE-CREATION 
>   ui/src/main/js/components/__tests__/TaskList-test.js PRE-CREATION 
>   ui/src/main/js/index.js 30646f78a422788c5dbc07f97ca61b739a339bd9 
>   ui/src/main/js/pages/Job.js PRE-CREATION 
>   ui/src/main/js/pages/__tests__/Job-test.js PRE-CREATION 
>   ui/src/main/js/test-utils/TaskBuilders.js 35b915217f67a712710473b5901d964f1675e4d9 
>   ui/src/main/js/utils/Task.js c58ff7d86af013bd977f4b114c3bffffe5f84aac 
>   ui/src/main/sass/app.scss 315b666bd4b56b6c0c1f5b53b38a9cd61a1e4b9c 
>   ui/src/main/sass/components/_job-page.scss PRE-CREATION 
>   ui/src/main/sass/components/_task-list.scss PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/62908/diff/2/
> 
> 
> Testing
> -------
> 
> ./gradlew ui:lint
> ./gradlew ui:test
> 
> See screenshots for live API testing.
> 
> 
> File Attachments
> ----------------
> 
> Simple job - no updates
>   https://reviews.apache.org/media/uploaded/files/2017/10/12/b6b53783-67d5-4cd8-a6ee-a9dc93f6f3e1__Screen_Shot_2017-10-11_at_5.16.48_PM.png
> Job With Update In Progress
>   https://reviews.apache.org/media/uploaded/files/2017/10/12/552bfba1-eb4d-4dfd-a21a-9d73d715230b__Screen_Shot_2017-10-11_at_5.18.29_PM.png
> Configuration Tab - with diffing
>   https://reviews.apache.org/media/uploaded/files/2017/10/12/6be8dc50-e231-4430-b3cf-7e383fc420bd__Screen_Shot_2017-10-11_at_5.19.25_PM.png
> Multi-Config Diffing support
>   https://reviews.apache.org/media/uploaded/files/2017/10/12/5bb4b753-af84-4fe1-9ce3-4110b2c96225__Screen_Shot_2017-10-11_at_5.20.15_PM.png
> Expanded task item with update history too
>   https://reviews.apache.org/media/uploaded/files/2017/10/12/7f2fc9e4-fa25-4b6b-ae50-afa90ac6992c__Screen_Shot_2017-10-11_at_5.20.39_PM.png
> 
> 
> Thanks,
> 
> David McLaughlin
> 
>


--===============4678234738266054219==--