[WIP] Fixes #25756 - Adds extend-ability to react components #6366

amirfefer · 2018-12-26T00:52:35Z

Work In Progress

This PR is written according to this design

In this demonstration I've added <Fill> component in katello subscription details page, and a <Slot> in the upper left bar:

        <Fill id="test">
          <Nav navbar pullLeft className="navbar-iconic">
            <Button style={{ marginTop: '15px' }} bsStyle="primary"> Extenable Button !</Button>
          </Nav>
        </Fill>

theforeman-bot · 2018-12-26T00:52:45Z

Issues: #25756

amirfefer · 2018-12-26T16:32:47Z

UPDATE: a slot can handle multiple Fills components, the rendering order will be by weights:

ohadlevy

Looks like a good start! I've added a few comments inline, plus there is an empty reducer file.

ohadlevy · 2018-12-27T07:20:29Z

webpack/assets/javascripts/react_app/components/common/Fill/ExtendableRegistery.js

+
+const components = {};
+
+export const registerComponent = (SlotId, key, component, weight) => {


would register/unregister (or add/remove etc) are enough? I think the component should already part of the context, e.g. when you are using it:

import { add, remove } from './ExtendableRegistery';

ohadlevy · 2018-12-27T07:20:49Z

webpack/assets/javascripts/react_app/components/common/Fill/ExtendableRegistery.js

+  }
+};
+
+export const getComponent = id =>


maybe worth comparing with the ruby implementation for menus e.g. at https://github.com/theforeman/foreman/blob/develop/app/registries/menu/manager.rb

ohadlevy · 2018-12-27T07:21:09Z

webpack/assets/javascripts/react_app/components/common/Fill/Fill.js

+class Fill extends React.Component {
+  componentDidMount() {
+    const { children, registerFillComponent, id, weight, fillId } = this.props;
+    debugger;


ohadlevy · 2018-12-27T07:21:27Z

webpack/assets/javascripts/react_app/components/common/Fill/Fill.js

+    removeRegisteredComponent(id, fillId);
+  }
+  render() {
+    return <div />;


glekner · 2018-12-27T16:19:44Z

This is amazing!

Would be great to have an option to give props to <Fill /> instead of JSX.
I guess this means every Slot will need to have a default Fill in it that can receive props for it to be customizable.

amirfefer · 2018-12-31T17:32:44Z

Great idea @glekner !
As you suggested, a Fill can contain JSX or plain object as props, with this logic plugins may totally override a slot or just modify the props.

Props way

// plugin
<Fill id='slot-id' fillId='some-id' overrideProps={{ text: 'this text given by a prop' }} />

// foreman core
const Wrapper = ({ text }) => <div>{text}</div>; 
<Slot id='slot-id'>
  <Wrapper text='some default text' />
</Slot>

JSX way

// plugin
<Fill id='slot-id' fillId='some-id'> 
  <div> some text </ div>
</ Fill>

// foreman core
<Slot id='slot-id'>
 default children // can be empty
</Slot>

xprazak2 · 2019-01-02T09:27:02Z

webpack/assets/javascripts/react_app/components/common/Fill/ExtendableRegistery.js

+
+export const remove = (SlotId, key) => {
+  if (components[SlotId]) {
+    components[SlotId].forEach((item, index) => {


nit: have you considered using filter?

filter creates a new array, while using splice the removing process is in place.
we can use filter and splice, having something like:

obj.splice(0, obj.length, ...obj.filter(item => item.key !== key))

sharvit

Awesome work @amirfefer 🥇

sharvit · 2019-01-03T08:42:24Z

webpack/assets/javascripts/react_app/components/common/Fill/Fill.js

+  componentWillUnmount() {
+    const { id, removeRegisteredComponent, fillId } = this.props;
+
+    removeRegisteredComponent(id, fillId);


nitpick: unregisterFillComponent?

sharvit · 2019-01-03T08:49:34Z

webpack/assets/javascripts/react_app/components/common/Slot/Slot.js

+    if (isValidElement(object)) {
+      return cloneElement(object, props);
+    }
+    return cloneElement(children, object);


shouldn't you keep the old props? return cloneElement(children, { ...props, ...object });

xprazak2 · 2019-02-06T13:35:43Z

webpack/assets/javascripts/react_app/components/common/Fill/index.js

+export const reducers = { extends: reducer };
+
+// export connected component
+export default connect(


should be added into component registry, same for Slot

houndci-bot · 2019-03-24T12:29:47Z

app/helpers/extendable_components_helper.rb

@@ -0,0 +1,7 @@
+module ExtendableComponentsHelper
+  def slot(slot_id, multi=false)


Layout/SpaceAroundEqualsInParameterDefault: Surrounding space missing in default value assignment.

xprazak2 · 2019-03-25T09:58:34Z

webpack/assets/javascripts/react_app/components/common/Fill/FillActions.js

@@ -0,0 +1,24 @@
+import { REGISTER_FILL, REMOVE_FILLED_COMPONENT } from './FillConstants';
+import { add, remove } from './ExtendableRegistery';


Google knows only the version without 'e', could you rename please?

xprazak2 · 2019-03-25T10:03:17Z

webpack/assets/javascripts/react_app/components/common/Fill/FillReducer.js

+
+    case REMOVE_FILLED_COMPONENT:
+      return state.setIn(
+        ['RegisteredComponent', payload.slotId, payload.fillId],


Same as comment on the other end.

xprazak2 · 2019-03-25T10:04:03Z

app/views/about/index.html.erb

 </div>
-</div>
+
+<%= slot('about-footer-slot', true) %>


Needs to be inside div#about tag, so it does not show all the way down:

waldenraines · 2019-04-17T01:47:42Z

@amirfefer can you rebase this so I can test it out please?

amirfefer · 2019-06-18T12:46:28Z

I'm closing this PR because I opened another one
#6849

theforeman-bot added Needs testing Not yet reviewed UI labels Dec 26, 2018

amirfefer force-pushed the 25756 branch from c504e11 to ed4b3df Compare December 26, 2018 16:27

ohadlevy reviewed Dec 27, 2018

View reviewed changes

amirfefer force-pushed the 25756 branch from ed4b3df to 7e2b26c Compare December 31, 2018 17:21

xprazak2 reviewed Jan 2, 2019

View reviewed changes

sharvit reviewed Jan 3, 2019

View reviewed changes

xprazak2 reviewed Feb 6, 2019

View reviewed changes

ezr-ondrej mentioned this pull request Feb 18, 2019

[WIP] Fixes #26078 - interface table in react #6490

Closed

tbrisker added the WIP label Mar 10, 2019

amirfefer mentioned this pull request Mar 10, 2019

Fixes #26287 - Add a loader of global js files for plugins #6554

Merged

amirfefer mentioned this pull request Mar 22, 2019

Fixes #26434 - move system statuses from deface to react Katello/katello#8039

Merged

amirfefer force-pushed the 25756 branch from 7e2b26c to 74af432 Compare March 24, 2019 12:29

houndci-bot reviewed Mar 24, 2019

View reviewed changes

Fixes #26287 - Add loader of global js for plugins

74af432

xprazak2 reviewed Mar 25, 2019

View reviewed changes

amirfefer closed this Jun 18, 2019


		const components = {};

		export const registerComponent = (SlotId, key, component, weight) => {

		@@ -0,0 +1,7 @@
		module ExtendableComponentsHelper
		def slot(slot_id, multi=false)

		@@ -0,0 +1,24 @@
		import { REGISTER_FILL, REMOVE_FILLED_COMPONENT } from './FillConstants';
		import { add, remove } from './ExtendableRegistery';

[WIP] Fixes #25756 - Adds extend-ability to react components #6366

[WIP] Fixes #25756 - Adds extend-ability to react components #6366

Uh oh!

Conversation

amirfefer commented Dec 26, 2018

Work In Progress

Uh oh!

theforeman-bot commented Dec 26, 2018

Uh oh!

amirfefer commented Dec 26, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ohadlevy left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

glekner commented Dec 27, 2018

Uh oh!

amirfefer commented Dec 31, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Props way

JSX way

Uh oh!

xprazak2 Jan 2, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sharvit left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

waldenraines commented Apr 17, 2019

Uh oh!

amirfefer commented Jun 18, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

amirfefer commented Dec 26, 2018 •

edited

Loading

amirfefer commented Dec 31, 2018 •

edited

Loading

xprazak2 Jan 2, 2019 •

edited

Loading