Skip to content
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

[WIP] injected angularjs dependencies #969

Open
wants to merge 2 commits into
base: react-migration
Choose a base branch
from

Conversation

gnehapk
Copy link
Member

@gnehapk gnehapk commented May 21, 2018

Problem - angularjs dependencies are getting injected after the code which read it.

In my code, statement const { $rootScope, $state, $q, $http } = ngDeps; inside event-store.js file is getting executed before the run module's callback, where we are injecting the dependencies(inside ng-react-ng-deps.js). Hence, the ngDeps is not getting updated with the dependencies. For the 1st time, when application gets displayed in browser, we can see the dependencies inside ngDeps(but still $q.defer is undefined) and if you refresh the page, ngDeps gets blank again because of the above mentioned issue.

Hence, for now required angularjs dependencies are added on window object and accessed via window object only.

@shirshendu ^^

@gnehapk gnehapk requested review from a team as code owners May 21, 2018 11:19
@gnehapk gnehapk force-pushed the migrate-to-react branch from 3b49289 to f789af3 Compare June 4, 2018 17:18
@gnehapk
Copy link
Member Author

gnehapk commented Jun 4, 2018

Completed following tasks-

  1. Injected angularjs dependencies
  2. Implemented Pub/Sub Model
  3. Implemented event list view in react

Pending -

  1. Filter in event list view
  2. Removing few dependencies(eg- $stateParams, config, utils) from window object if possible(need to check)
  3. Some css pending which are based on conditions in event list view

@shirshendu Please review.

@gnehapk gnehapk force-pushed the migrate-to-react branch from f789af3 to 5b7265e Compare June 4, 2018 17:23
//url = "/api/events.json";
url = config.baseUrl + "clusters/" + clusterId + "/notifications";
url = "/api/events.json";
//url = config.baseUrl + "clusters/" + clusterId + "/notifications";

Choose a reason for hiding this comment

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

I thought this change only helps with testing. Should it be here?

Copy link
Member Author

@gnehapk gnehapk Jun 12, 2018

Choose a reason for hiding this comment

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

@shirshendu Yes, its just for testing. I will remove it.

@gnehapk gnehapk force-pushed the migrate-to-react branch from 5b7265e to f4e14eb Compare June 12, 2018 11:08
2) Implemented Pub/Sub Model
3) Implemented event list view in react

Signed-off-by: Neha Gupta <[email protected]>
@gnehapk gnehapk force-pushed the migrate-to-react branch from f4e14eb to db6cd72 Compare June 12, 2018 11:37
@gnehapk
Copy link
Member Author

gnehapk commented Jun 12, 2018

@shirshendu I have amended the PR. Please review.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants