Skip to content

[DICOM Archive] Fully react-rendered + written in ES6 #2397

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 10 commits into from
Nov 25, 2016

Conversation

alisterdev
Copy link
Contributor

@alisterdev alisterdev commented Nov 3, 2016

🎉 New cool features below 🎉

  • Updated dicom_archive as an example module written in ES6
  • Simplified code for the Filter, removed unnecessary state
  • Completely moved page rendering to Javascript

Note 1
In order for changes above to work, had to update QueryString API, and include it globally.
Note 2
Sorry for large amount of changed files @jacobpenny
Note 3
This update relies on #2433, so it shouldn't be merged before 2433

@alisterdev alisterdev added Category: Feature PR or issue that aims to introduce a new feature Request Code Review labels Nov 3, 2016
@alisterdev alisterdev added this to the 17.1 milestone Nov 3, 2016
/* global QueryString */

const propTypes = {};
const defaultProps = {};
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Might as well define these now

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jacobpenny The default properties are page specific

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah gotcha, makes sense.

@stellarxo stellarxo added the Passed manual tests PR has been successfully tested by at least one peer label Nov 9, 2016
@jacobpenny
Copy link
Contributor

I think it is worth reconsidering the use of inheritance here because it isn't idiomatic in the React world. Facebook has a pretty strong opinion on this: https://facebook.github.io/react/docs/composition-vs-inheritance.html#so-what-about-inheritance

Also summed up nicely by one of the core React contributors here:
facebook/react#613 (comment)

@alisterdev alisterdev added State: Needs work PR awaiting additional work by the author to proceed and removed Request Code Review labels Nov 22, 2016
@alisterdev alisterdev force-pushed the 161027_react_loris_page branch from 664d6ad to f337b83 Compare November 22, 2016 22:40
@alisterdev
Copy link
Contributor Author

alisterdev commented Nov 22, 2016

@jacobpenny Scratched the whole inheritance idea as per your suggestion and simplified filter implementation instead.

@alisterdev alisterdev added Request Code Review and removed State: Needs work PR awaiting additional work by the author to proceed labels Nov 22, 2016
Copy link
Contributor

@jacobpenny jacobpenny left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me!

@alisterdev alisterdev changed the title [Core] React LorisPage [DICOM Archive] Fully react-rendered + written in ES6 Nov 24, 2016
@driusan driusan merged commit 47ae009 into aces:17.1-dev Nov 25, 2016
alisterdev added a commit to alisterdev/Loris that referenced this pull request Jan 20, 2017
* Add LorisPage component to Loris

* Update dicom_archive to use LorisPage

* Remove unused statement

* PHPCS

* Remove LorisPage and move behaviour back to module

* Update querystring API for less code and more pleasant experience

* Add QueryString to global eslint ignore list

* Clear tpl as everything is rendered in react now

* Update mri_violation to use new QueryString API

* Replace DynamicTable with StaticTable since all data is loaded in the parent component
MounaSafiHarab pushed a commit to MounaSafiHarab/Loris that referenced this pull request Jan 20, 2017
* Add LorisPage component to Loris

* Update dicom_archive to use LorisPage

* Remove unused statement

* PHPCS

* Remove LorisPage and move behaviour back to module

* Update querystring API for less code and more pleasant experience

* Add QueryString to global eslint ignore list

* Clear tpl as everything is rendered in react now

* Update mri_violation to use new QueryString API

* Replace DynamicTable with StaticTable since all data is loaded in the parent component
taracampbell pushed a commit to taracampbell/Loris that referenced this pull request Mar 9, 2017
* Add LorisPage component to Loris

* Update dicom_archive to use LorisPage

* Remove unused statement

* PHPCS

* Remove LorisPage and move behaviour back to module

* Update querystring API for less code and more pleasant experience

* Add QueryString to global eslint ignore list

* Clear tpl as everything is rendered in react now

* Update mri_violation to use new QueryString API

* Replace DynamicTable with StaticTable since all data is loaded in the parent component
jstirling91 pushed a commit to jstirling91/Loris that referenced this pull request May 8, 2017
* Add LorisPage component to Loris

* Update dicom_archive to use LorisPage

* Remove unused statement

* PHPCS

* Remove LorisPage and move behaviour back to module

* Update querystring API for less code and more pleasant experience

* Add QueryString to global eslint ignore list

* Clear tpl as everything is rendered in react now

* Update mri_violation to use new QueryString API

* Replace DynamicTable with StaticTable since all data is loaded in the parent component
kongtiaowang pushed a commit to kongtiaowang/Loris that referenced this pull request Jan 31, 2018
* Add LorisPage component to Loris

* Update dicom_archive to use LorisPage

* Remove unused statement

* PHPCS

* Remove LorisPage and move behaviour back to module

* Update querystring API for less code and more pleasant experience

* Add QueryString to global eslint ignore list

* Clear tpl as everything is rendered in react now

* Update mri_violation to use new QueryString API

* Replace DynamicTable with StaticTable since all data is loaded in the parent component
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Category: Feature PR or issue that aims to introduce a new feature Passed manual tests PR has been successfully tested by at least one peer
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants