Move componentRegistry, mount, mount_react_component to v2v #334
Conversation
|
will await #331 resolution |
package.json
Outdated
| @@ -64,6 +64,8 @@ | |||
| "file-saver": "^1.3.8", | |||
| "intl": "~1.2.5", | |||
| "jquery": "~3.2.1", | |||
| "lodash.foreach": "^4.5.0", | |||
| "lodash.map": "^4.6.0", | |||
There was a problem hiding this comment.
is there any reason why to continue using lodash map and lodash foreach? I believe there are es6 compatible versions of those methods..
https://github.com/you-dont-need/You-Dont-Need-Lodash-Underscore#_each
https://github.com/you-dont-need/You-Dont-Need-Lodash-Underscore#_map
There was a problem hiding this comment.
orderby is not so simple though and used by our sortabular/table sorting component ;(
There was a problem hiding this comment.
Good point, probably no reason at all for the 2 :)
himdel
changed the title
|
Fixed the lodash imports, but rebased on top of #331 so I'll have to rebase again once that one is in :) (hence the WIP) |
|
ok... now #331 has been merged... rebase and some minor eslint fixes it looks like... |
himdel
changed the title
not being used in ui-classic, there's a new registry being rolled out now, less interdependencies if we just keep it here
no point for iterating over an array, or getting keys of a hash
…n running in specs
|
@himdel So the idea behind this PR to avoid backporting a ton of other PRs in ui-classic to gaprindashvili? |
|
since this creates a local component registry (and no longer references I would like to revisit this pattern in the future if we can. I think we may be able to achieve module separation using webpack code splits / ES6 JS modules instead of the global cache. I'm still not accustom to this sort of pattern and not sure what other teams are going w/ here. |
|
for the short term (i.e. for upcoming releases) - i have no issue w/ this at all. Just let us know if you'd like this in |
|
tested this locally.. it works fine for me. |
|
So.. basically, last week, we finished setting up the new registry in ops UI - that one is now documented in guides. The idea is to use the new registry instead - but mostly only for the root component - essentially any time you need to render a react component in non-react context. The new registry can work with any react component directly - dropping the need for components to take only So... I'm moving the old registry here, as right now, v2v completely depends on it right now.
Yup, a ton :). And not to break it when we remove it.
Yup, the idea behind it being local is that you're free to change it to something more standard (or not, up to you) :). (And you'll know you can't break any other plugin by doing that.)
Both gaprindashvili and master please :). |
Ok, this is making more sense now. From an MIQ standpoint supporting multiple frontend frameworks, I can see the case for this on adding React support to the legacy Ng pages. This is not really necessary in a React only architecture I suppose.
With a lot of product teams moving to consuming PatternFly React and those components becoming standardized and more scalable directly from PF, I hope this need diminishes, but I see your point here. I tend to prefer consuming directly from PF for new React pages unless we are extending a component for a specific need across MIQ. In that case we have the ManageIQ/react-ui-components repo. I.m.o. we should try to make the upstream PF React components as flexible as possible first though.
Not a problem. I would like to reconsider this topic post release. Thanks for your patience. |
|
@AparnaKarve do you have any more concerns before merge? |
|
Verified locally that these changes work. |
Not being used in ui-classic, there's a new registry being rolled out now,
less interdependencies if we just keep it here.
ManageIQ/manageiq-ui-classic#3963: needed because no registry in gaprindashvili ui-classic
(This PR is based on #331, merge that one first please)