Mailing-List: contact reviews-help@aurora.apache.org; run by ezmlm
Precedence: bulk
Reply-To: reviews@aurora.apache.org
Content-Type: multipart/alternative;
 boundary="===============1955776117714832169=="
MIME-Version: 1.0
Subject: Re: Review Request 62720: Implement Instance pages 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>, Stephan Erb <serb@apache.org>,
 Joshua Cohen <jcohen@apache.org>, David McLaughlin <david@dmclaughlin.com>,
 Reza Motamedi <reza.motamedi@gmail.com>, Aurora <reviews@aurora.apache.org>
Date: Thu, 05 Oct 2017 23:50:54 -0000
Message-ID: <20171005235054.52732.45019@reviews-vm2.apache.org>
Auto-Submitted: auto-generated
Sender: David McLaughlin <noreply@reviews.apache.org>
References: <20171005002857.34866.8091@reviews-vm2.apache.org>
In-Reply-To: <20171005002857.34866.8091@reviews-vm2.apache.org>
Reply-To: David McLaughlin <david@dmclaughlin.com>
archived-at: Thu, 05 Oct 2017 23:51:00 -0000

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


> On Oct. 5, 2017, 12:28 a.m., Kai Huang wrote:
> > ui/src/main/js/components/StateMachine.js
> > Lines 37 (patched)
> > <https://reviews.apache.org/r/62720/diff/2/?file=1845625#file1845625line37>
> >
> >     Add a key property here to suppress the warnings in the browser console?

Done.


- David


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


On Oct. 5, 2017, 11:50 p.m., David McLaughlin wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62720/
> -----------------------------------------------------------
> 
> (Updated Oct. 5, 2017, 11:50 p.m.)
> 
> 
> Review request for Aurora, Kai Huang and Santhosh Kumar Shanmugham.
> 
> 
> Repository: aurora
> 
> 
> Description
> -------
> 
> Instance page moved over to the new UI. 
> 
> One of the big issues I ran into here was dealing with Thrift types. Currently the Thrift compiler generates a file for use in JS. The compiler gives you two options for JavaScript - one is 'node' which plays well with our module system but assumes you'll be using the built-in socket implementation in node, and then one for 'jquery' which uses the global namespace but doesn't use vars (so we can't eval as it breaks strict mode). So the TL;DR is - to write tests against scheduler states, I've had to copy and paste the Thrift enums into a test setup file. 
> 
> The alternative is to simply ignore Thrift types and duplicate it all without relying on globals. Thus making sure the tests and application code are using the same enums. However, this approach won't help you find any UI regressions when you update Thrift - and even worse, you won't spot any API regressions when developing the UI. So I've just gone for this approach for now.
> 
> 
> Diffs
> -----
> 
>   ui/.eslintrc 8d37c60e1edd4528ce4abdf6dda18ec2dfc078f4 
>   ui/package.json fe0397a888b337137c1d13b6f9559dae13698b0a 
>   ui/src/main/js/components/InstanceHistory.js PRE-CREATION 
>   ui/src/main/js/components/InstanceHistoryItem.js PRE-CREATION 
>   ui/src/main/js/components/Layout.js 4ca54e318786859e23dc0444d7d2750817428e81 
>   ui/src/main/js/components/StateMachine.js PRE-CREATION 
>   ui/src/main/js/components/TaskDetails.js PRE-CREATION 
>   ui/src/main/js/components/TaskStatus.js PRE-CREATION 
>   ui/src/main/js/components/__tests__/InstanceHistory-test.js PRE-CREATION 
>   ui/src/main/js/components/__tests__/InstanceHistoryItem-test.js PRE-CREATION 
>   ui/src/main/js/components/__tests__/StateMachine-test.js PRE-CREATION 
>   ui/src/main/js/index.js 4a879e6163eaabc203f2d3133419438c4e9730e8 
>   ui/src/main/js/pages/Instance.js PRE-CREATION 
>   ui/src/main/js/pages/__tests__/Instance-test.js PRE-CREATION 
>   ui/src/main/js/test-utils/Builder.js PRE-CREATION 
>   ui/src/main/js/test-utils/TaskBuilders.js PRE-CREATION 
>   ui/src/main/js/test-utils/__tests__/Builder-test.js PRE-CREATION 
>   ui/src/main/js/utils/Common.js 2e12e3cc6af75a62de9f11797f89dbf3279d2ce6 
>   ui/src/main/js/utils/Task.js PRE-CREATION 
>   ui/src/main/js/utils/Thrift.js PRE-CREATION 
>   ui/src/main/sass/app.scss d9673d0e8faa0b565f92e068ed4e01de0c789e8d 
>   ui/src/main/sass/components/_instance-page.scss PRE-CREATION 
>   ui/src/main/sass/components/_job-list-page.scss d31344d8eabb41114f4ff4678ff841119dfdac82 
>   ui/src/main/sass/components/_layout.scss 1d0553b8fdee2c9e11727831b7a112f534ce3ec6 
>   ui/src/main/sass/components/_state-machine.scss PRE-CREATION 
>   ui/src/main/sass/components/_status.scss PRE-CREATION 
>   ui/test-setup.js 054e7c2302aa70bc94a18581af5bd8e4e70559b1 
> 
> 
> Diff: https://reviews.apache.org/r/62720/diff/3/
> 
> 
> Testing
> -------
> 
> ./gradlew ui:lint
> ./gradlew ui:test
> 
> Testing in Vagrant.
> 
> 
> File Attachments
> ----------------
> 
> Active Task with State Machine component
>   https://reviews.apache.org/media/uploaded/files/2017/10/04/16f93047-a94b-4ffa-92f2-7fc27dbed9c9__Screen_Shot_2017-10-03_at_10.12.34_PM.png
> Expanded Finished Task
>   https://reviews.apache.org/media/uploaded/files/2017/10/04/f5daaf35-4e4b-4106-b7ce-2773b97d2277__Screen_Shot_2017-10-03_at_10.28.18_PM.png
> Tooltip
>   https://reviews.apache.org/media/uploaded/files/2017/10/04/0fe652c3-48a3-4292-8179-7d09bfbcc1e4__Screen_Shot_2017-10-03_at_10.28.33_PM.png
> 
> 
> Thanks,
> 
> David McLaughlin
> 
>


--===============1955776117714832169==--