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

Updates to Sources/Scans UI views #470

Merged
merged 1 commit into from Jan 23, 2018

Conversation

jeff-phillips-18
Copy link
Contributor

@jeff-phillips-18 jeff-phillips-18 commented Jan 22, 2018

updates #434

@coveralls
Copy link

coveralls commented Jan 22, 2018

Coverage Status

Coverage remained the same at 90.857% when pulling 6f8db3e on jeff-phillips-18:issues/434 into 94a354c on quipucords:master.

Copy link
Member

@cdcabrera cdcabrera left a comment

Choose a reason for hiding this comment

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

Going ahead and approving, found a fix for unit test, if you're curious...
facebook/create-react-app#3482 (comment)

Once this stuff is in, I'll go ahead and make the noted mod.

</Router>
</Provider>,
div
);
Copy link
Member

Choose a reason for hiding this comment

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

This took a bit of search... so part of the issue with these components is we have an async aspect, hence one of the problems with Fetch (not the non-existent finally method). Adding this React.unmountComponentAtNode(div) after line 19 ensures the initial mount and immediate unmount of the component for a quick test, soooo like so:

ReactDOM.render(
  <Provider store={store}>
    <Router>
      <App />
    </Router>
  </Provider>,
  div
);

ReactDOM.unmountComponentAtNode(div);

If we combine this with converting the finally to a then or simply removing them. The tests should all pass with just a general console warning about the redirect... which we can address later.

Found this here... facebook/create-react-app#3482 (comment)

message: 'Adding sources is not yet implemented'
});
}

Copy link
Member

Choose a reason for hiding this comment

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

Good first round!

Probably a concept we have to evolve since it's hard to visualize, combining these dispatches into grouped actions ... so in the end, maybe something like

runScans()  {
  Store.dispatch({
    type: scansTypes.DO_ALL_THINGS_FOR_RUN_SCANS,
    ...

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.

None yet

3 participants