VisibleComponent - why do we need it?

[weskamm]

Why do we need a VisibleComponent?
I think that detection if a component should be rendered or not is part of the render function in the specific parent. This would also lead to a performance gain.
Applications should handle that on their own - or use a util for this.
Besides, using a prop named activeModules is a very special thingy coming from a project and should not be included in this library.
Thoughts @terrestris/devs ?

[dnlkoch]

Actually VisibleComponent is just a HOC wrapper that checks if a component should be rendered (inside the render method obviously) or not and the main purpose of this component is to share the is-component-to-render code so projects don't have to repeatedly implement it. I don't think there is any project related stuff in it even if the name of the prop activeModules has it's origin in a specific project, but this could be changed for sure.

[KaiVolland]

But couldn't you just solve this with the following?

{
  activeModules.includes('ImportButton') ? <ImportButton /> : null
}

[dnlkoch]

Yes, absolutely!

But if you don't want to repeat it in your application several times, e.g. if it has to check it for n buttons in a group, you could use the HOC.

[KaiVolland]

Indeed it seems to look a little nicer, in the render method itself:

const VisibleSimpleButton = isVisibleComponent(SimpleButton);

…

render(
  <VisibleSimpleButton
    activeModules={activeModules}
  />
)

The ternery syntax is more clear to me and it feels more reactish. Maybe just a matter of taste.

[weskamm]

Reason why i brought attention to this:
I think its a bit overkill to instantiate a new React.Component to check if a button should be rendered.
Also, the activeModules ternery may be a bit misleading Would be the same as creating a new HOC handling the initiallyActive flag of buttons.
Code like this could be handled in application utils itself, but its just my opinion.
Feel free to close the issue

[dnlkoch]

You're right about the React.Component, we should try (at least) to extend React.PureComponent here.

[marcjansen]

Hrmm, can we make use of React.PureComponent? Did anybody test?

[dnlkoch]

Closing this one, please refer to #650 for the usage of PureComponent.