MoroccanOSS / react-stoon

react-stoon is a handy toolbox of reusable Components that allow you to reduce boilerplate by writing less js inside of your React Components.
MIT License
29 stars 10 forks source link

Show controls my code & only renders React Components #11

Closed elhoucine closed 6 years ago

elhoucine commented 6 years ago

Hi! nice idea 👍 🥇

I tried the Show helper and here are some thoughts:

Show only work for React components, For example, this will not work:

<Show $={"img"} if={imageLink != ""}>
  <img src={imageLink} />
</Show>

And while it works well for Components, the Show syntax mixes with the application logic 😢 For example, JS conditional:

{ 
  project && <ProjectItem onDelete={this.deleteProject} project={project} />
}

After applying Show helper:

<Show
  $={ProjectItem}
  if={project}
  onDelete={this.deleteProject}
  project={project}
/>

In the code above Show dominated the component & it's props 😈 instead, we could have this:

<Show if={project} />
  <ProjectItem onDelete={this.deleteProject} project={project} />
</Show>

In the last syntaxe Show will only decide if or not to render, without taking care of props or other stuff.

It would be cool if Show works only as a conditional helper & for both React and Dom elements.

Let me know what you think.

Amine-H commented 6 years ago

Hey, as for the dom elements, react-stoon would need a wrapper for the component for it to work, something like:

const Img = (props) => <img ...props />;
<Show $={Img} if={imageLink != ""}>
  <Img src={imageLink} />
</Show>

which is basically a limitation or a bug to fix in a future version.

as for your second point, I agree it's a little bit confusing, but actually pretty concise. the most important thing is that the user would enjoy using these helpers that's why choosing either approach is very important, we would need to host a poll for which one to go for in the next releases, and if we are to support both how could we do it? maybe if $ is null then behave as suggested?

let's keep this open until we decide on this.

elhoucine commented 6 years ago

Hi @Amine-H,

Let me know when you host a poll I'll be happy to participate.

Amine-H commented 6 years ago

Hey, @elhoucine

following our discussion and considering that the poll resulted in favor of the proposed approach, we'll tag this Issue in the commit when merging it.

Thanks for your collaboration!

Links https://web.facebook.com/groups/137841990252945/permalink/162664217770722/