Use ui classic redux and router #533

Hyperkid123 · 2018-08-03T08:54:37Z

In order to fix ManageIQ/manageiq-ui-classic#4361 (comment). It is required to merge v2v router and redux with ui-classic.

Because ui-classic cannot use BrowserRouter (mainly because it would take an insane amount of time), we were forced to use hashRouter. Additionally, most of the screens, are controlled via toolbars, trees and other controls, that are not part of most of the components it was also required to use redux to handle some routing. This creates conflict between ui-classic routing and v2v routing.

Fortunately it can be fixed fairly easily.

PR dependencies in ui-classic

Necessary changes

use one redux store for react
use same routing as in ui-classic
this will not break current v2v routing. Only change is in the URL. It will start use hash routing, but the react-router-dom Links will be unaffected. Only the route must be adapted by removing the /migration prefix from them.
Enable manageIQ as a global variable
Use ui-classic mounting method
this is conflicting with the mounter from v2v. I have also found out, that the mounter is specifying some default prop values. I personally think this is not a good decision and these values should be actually default props. Or if this was meant to be configurable, just move them to redux store, or wrap them into object and pass them as a input prop to root component and distribute them from there. If there is some other reason for this to exist, please let me know a will try to find some solution, that will keep this unchanged.

remove the app/javascript/components/index.js for now i have moved these default props to corresponding components (mainly the index.html.hml, but it is not pretty and we should try to find some other place to put them)

Test are green and from what i can tell, it works and fixes the error from the comment above. But this is the first time i see the v2v component, so somebody elso should go through it and try to find out if i forget something.

Hyperkid123 · 2018-08-03T08:56:08Z

cc @michaelkro @priley86

Hyperkid123 · 2018-08-03T09:01:27Z

cc @skateman @himdel

skateman

Oh my, it's javascript... :scared:

skateman · 2018-08-03T09:40:36Z

app/assets/stylesheets/manageiq-v2v/base.scss

+
+.react-wrapper {
+  height: 100%;
+}


Missing newline ;)

michaelkro · 2018-08-03T12:13:58Z

It looks like it was always the intention to use ManageIQ.redux.addReducer and ManageIQ.redux.store

Issue: #17

AllenBW · 2018-08-03T16:45:07Z

Because its relevant, discussion about this also happened here.

priley86 · 2018-08-06T14:41:26Z

hi @Hyperkid123 - apologies for late response (I am returning from extended PTO 😄 ).

Thanks for revisiting this one (and thanks to @michaelkro and @AllenBW for keeping this moving!). I have no issues at all with introducing connected-react-router if that is consistent w/ redux routing in MIQ. It seems like you may be missing some of the benefits of that library though after taking a quick glance. I will post a couple of comments inline and see what you think...this looks like a good direction though!

priley86 · 2018-08-06T14:43:41Z

app/javascript/react/screens/App/Overview/components/Migrations/MigrationsCompletedList.js

@@ -189,7 +189,7 @@ class MigrationsCompletedList extends React.Component {
                        onClick={e => {
                          e.stopPropagation();

-                          redirectTo(`/migration/plan/${plan.id}`);
+                          redirectTo(`/plan/${plan.id}`);


do you not want to use the connected react router's push method now? This should enable the history to be tracked in redux state.

Yep i know about this. My mindset about this is PR, is to make as little changes as possible (we use the push, goBack etc. methods in ui-classic) to minimize chance of breaking something.

But if you will require these changes i will happily do it

@Hyperkid123 i think it would be a nice to test this feature if it's not too much additional (otherwise, yes, fine w/ waiting).

I will try to pull this branch down and do some more testing on my side soon...

OK i will do it, its very simple change anyway. The worst thing will be finding all the places where it should be used 😄 .

priley86 · 2018-08-06T14:47:14Z

app/javascript/react/index.js

-    </Router>
-  </Provider>
-);
+class OwerView extends React.Component {


did you mean to call this "Overview" or something else? I guess since this is the root of the application (and not technically the Overview), maybe we can just call this class "Index" ? just a consideration...

priley86 · 2018-08-06T15:22:52Z

.../screens/MappingWizard/components/MappingWizardDatastoresStep/MappingWizardDatastoresStep.js

@@ -151,7 +151,11 @@ MappingWizardDatastoresStep.propTypes = {
 };
 MappingWizardDatastoresStep.defaultProps = {
  clusterMappings: [],
-  fetchStoragesUrls: {},
+  fetchStoragesUrls: {


if this is the alternative to using the data props in app/javascript/components/index.js I am 100% OK with this. That is just an artifact of the original way to use the component registry (which now seems dated). This method should be easier to follow and preferred now it seems...

I figured it was used for configuration of the plugin. So if there is scenario, when it might be required to change those values (like using it in different product) i would maybe recommend to move them to redux and have some "read only" reducer for configuration. I think all components that are using this are connected anyway, so basically only the origin of the data would change. We would not have move the data to defaultProps and we could eventually delete the whole coreComponents file.

Only other thing that is depending on this are the routes i think, but it should not be hard to replace.

@priley86 ok i will move it to default props for now. You guys can decide which path you want to take later. Also ,removing the original component registry should not be part of this PR. That is maybe something for future refactoring.

himdel · 2018-08-09T10:23:41Z

jest: testURL
This is a bit weird, sometimes it shows sometimes it doesn't, we had a similar issue here
ManageIQ/react-ui-components#67

Same in ManageIQ/manageiq-ui-classic#4371, this depends on the version of jsdom being used.

priley86 · 2018-08-09T13:14:16Z

@Hyperkid123 some background on react-intl can be found here...

Initially in the project we were not sure how to best add lanaguage/date/time internationalization support in React and decided to follow this Foreman PR. However, it looks like Foreman did not move forward with this.

Since we are using gettext and happy w/ that for messages, and now using Moment JS for dates/time, it seems like we can safely remove react-intl.

@AllenBW @AparnaKarve - any thoughts on this?

AllenBW · 2018-08-09T13:54:37Z

@priley86 I agree. 😏 🍶 😋 👍

AparnaKarve · 2018-08-09T13:57:53Z

@priley86 Yes, OK to remove react-intl

priley86 · 2018-08-09T14:03:24Z

@priley86 I agree. 😏 🍶 😋 👍

ok - opened #559 for this @AllenBW . 🌮 💃

Hyperkid123 · 2018-08-09T15:52:44Z

@priley86 ok, i was just asking because i did not add the intl context to the component.

Hyperkid123 · 2018-08-15T06:26:30Z

@priley86 @AllenBW is there something else that i should do?
btw travis is failing with node version 6

error typescript-eslint-parser@16.0.1: The engine "node" is incompatible with this module. Expected version ">=6.14.0".
error Found incompatible module

Did you add something recently?

AllenBW · 2018-08-15T11:09:25Z

Nah nothing of that nature was recently added @Hyperkid123, looking into it... maybe we'll just 🗑 6.10 tests 🤔

himdel · 2018-08-15T11:17:38Z

Oh yeah, sorry about that, looks like we don't support node 6 anymore... (Right now, this is just a ... in our webpack config, so we can undo that if there's a problem .. but the build is using node 8 since last week.)

AllenBW · 2018-08-15T11:20:11Z

Hmmm so yah think we can can all travis tests that run on node 6? (here and also SUI?)

himdel · 2018-08-15T11:34:11Z

can can 💃 :D But yeah, can all the tests :)

AllenBW

Okie dokie, I'm ok with this, nothing breaks 🙇 a flippin awesome undertaking, thanks for the contribution @Hyperkid123 !!

@michaelkro @AparnaKarve ya'll wanta weigh in here before a merge? (we're ignoring the failing travis test)

michaelkro

Took it for a spin, and everything looks good. Thank you for your time and effort on this @Hyperkid123!

Hyperkid123 · 2018-08-15T14:48:42Z

Np, happy to help

AparnaKarve · 2018-08-15T17:38:11Z

Tested this and seems to work well.

It looks like with this change, we won't see the redux logging in the console anymore.

We do have the Redux Dev tools Chrome extension to monitor the redux activity, but I do like the console logging better. Can we enable that if possible?

Other than the above observation, everything else looks great!
Thanks @Hyperkid123 !

AllenBW · 2018-08-16T10:37:50Z

Gonna merge this before some really 🤕 merge conflicts start popping up.. @AparnaKarve totally not bailing on the need to console log redux activity! Can be done in another pr 🤗

martinpovolny · 2018-08-17T13:34:58Z

Happy to see this landed! 🎆 Thanks to all for the effort 🍻

May I suggest the V2V team to ping @himdel, @skateman, @Hyperkid123 and @martinpovolny in case you are about to introduce some major concept in V2V. I mean something like routing, redux store, notifications or i18n. Something that has a big potential of clashing with the ui-classic. Please?

It's absolutely not that I'd want to slow you down in your great job. I'd like the cooperation to be as smooth as possible.

michaelkro · 2018-08-17T13:58:50Z

Yes, we definitely will @martinpovolny! 🍻

Hyperkid123 force-pushed the use-ui-classic-redux-and-router branch from 0ad4d49 to 0afc6b8 Compare August 3, 2018 09:18

skateman suggested changes Aug 3, 2018

View reviewed changes

Hyperkid123 force-pushed the use-ui-classic-redux-and-router branch 2 times, most recently from 6802c34 to 91138d1 Compare August 3, 2018 10:24

AllenBW requested review from mturley, AparnaKarve, AllenBW and michaelkro August 3, 2018 12:35

AllenBW added bug gaprindashvili/no labels Aug 3, 2018

AllenBW added this to In progress in v2v UI via automation Aug 3, 2018

AllenBW added the wip label Aug 3, 2018

priley86 reviewed Aug 6, 2018

View reviewed changes

Hyperkid123 force-pushed the use-ui-classic-redux-and-router branch from 0b6ba18 to f33a38f Compare August 7, 2018 11:12

Hyperkid123 added 7 commits August 7, 2018 14:18

Use manageiq redux store.

3c9b720

Use manageiq mounting for react component

a98bd9d

Removed migration prefix from router links.

fd23397

Moved missing props from mounter to default props.

2e73747

Enable ManageIQ as a global variable.

ae1414b

Replaced redirectTo with routing redux action.

0608470

Renamed root component.

32ff408

Hyperkid123 force-pushed the use-ui-classic-redux-and-router branch 2 times, most recently from ddd4129 to 6e7ffef Compare August 7, 2018 13:50

priley86 mentioned this pull request Aug 9, 2018

Remove react-intl #559

Closed

Hyperkid123 closed this Aug 11, 2018

Hyperkid123 reopened this Aug 11, 2018

AllenBW approved these changes Aug 15, 2018

View reviewed changes

This was referenced Aug 15, 2018

Removes unsupported node 6 test env, updates yarn ManageIQ/manageiq-ui-service#1470

Merged

Updates travis node targets to reflect miq supported versions #575

Merged

michaelkro approved these changes Aug 15, 2018

View reviewed changes

AllenBW removed the wip label Aug 15, 2018

AllenBW merged commit d72a235 into ManageIQ:master Aug 16, 2018

v2v UI automation moved this from In progress to Done Aug 16, 2018

michaelkro mentioned this pull request Aug 17, 2018

[#546][2 of (3)?] OSP Support: Add editable Instance Properties table to Plan Wizard #562

Merged

8 tasks

This was referenced Nov 8, 2018

Removed old react mounter. ManageIQ/manageiq-ui-classic#4886

Merged

Remove outdated mount_react_component. #779

Merged

michaelkro mentioned this pull request Dec 14, 2018

Integrate Redux store / reducers w/ mIQ Core #17

Closed

Use ui classic redux and router #533

Use ui classic redux and router #533

Conversation

Hyperkid123 commented Aug 3, 2018 • edited Loading

PR dependencies in ui-classic

Necessary changes

Hyperkid123 commented Aug 3, 2018

Hyperkid123 commented Aug 3, 2018

skateman left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michaelkro commented Aug 3, 2018 • edited Loading

AllenBW commented Aug 3, 2018

priley86 commented Aug 6, 2018

Choose a reason for hiding this comment

Hyperkid123 Aug 6, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

himdel commented Aug 9, 2018

priley86 commented Aug 9, 2018

AllenBW commented Aug 9, 2018

AparnaKarve commented Aug 9, 2018

priley86 commented Aug 9, 2018

Hyperkid123 commented Aug 9, 2018

Hyperkid123 commented Aug 15, 2018

AllenBW commented Aug 15, 2018

himdel commented Aug 15, 2018

AllenBW commented Aug 15, 2018

himdel commented Aug 15, 2018

AllenBW left a comment

Choose a reason for hiding this comment

michaelkro left a comment

Choose a reason for hiding this comment

Hyperkid123 commented Aug 15, 2018

AparnaKarve commented Aug 15, 2018

AllenBW commented Aug 16, 2018

martinpovolny commented Aug 17, 2018 • edited Loading

michaelkro commented Aug 17, 2018

Hyperkid123 commented Aug 3, 2018 •

edited

Loading

michaelkro commented Aug 3, 2018 •

edited

Loading

Hyperkid123 Aug 6, 2018 •

edited

Loading

martinpovolny commented Aug 17, 2018 •

edited

Loading