deprecate / metal-clay-components

10 stars 14 forks source link

Separate ClayAlert, ClayStripe and ClayToast #160

Closed carloslancha closed 6 years ago

matuzalemsteles commented 6 years ago

Just started reviewing :)

:octocat: Sent from GH.

matuzalemsteles commented 6 years ago

Hey @carloslancha, why separate ClayAlert? I think it gets confusing when the most files. What do you think of separating folders into src with their component names?

Example:

.
+-- src
|   +-- index.js
|   +-- alert/
|   +-- alert-stripe/
|   +-- alert-toast/

We could use index.js to save the imports of each main file.

// src/alert/index.js

import ClayAlert from './ClayAlert';

export default ClayAlert;

This gives us more flexibility to architect more files within each folder if it grows in the future and I see it as a more elegant way in imports.

Example:

// src/index.js

import ClayAlert from 'alert';
import ClayStripe from 'clay-stripe';
import ClayToast from 'clay-toast';

....

What do you think about this ?

jbalsas commented 6 years ago

Hey @matuzalemsteles, we already have this pattern and I assume this PR follows it. Take a look at clay-dropdown/src/all.js

For multi-export packages, that's the pattern we follow. It allows us to do:

import ClayAlert from 'clay-alert';
import {ClayToast} from 'clay-alert';

That's the thing we need to keep for sure, since all.js is the entry point that will get consumed by bundlers and other tooling.

I'm not really for or against moving files around, but for now I'd stick with what we have and reconsider later on 😉

matuzalemsteles commented 6 years ago

Hey @jbalsas, I understand. Thanks for the clarification.

We could continue with this pattern by just leaving the clay-alert,, clay-stripe, clay-toast components in their respective folders. Keeping all.js insrc root. 🙂

// src/all.js

import ClayStripe from './clay-stripe';

export {ClayStripe};

We still have the same pattern in the final exit.

screen shot 2017-11-21 at 14 55 25

Not necessarily with those names.

screen shot 2017-11-21 at 15 02 45

just leaving my vision. 🙂

jbalsas commented 6 years ago

Hey @matuzalemsteles, I'm not against it, but we have inherited this pattern from metal.js so we should follow for now. I'm fine bringing in @Robert-Frampton as well in case he wants to reconsider so we can apply this consistently, but I'm not sure how much better this is... as I grow old, I tend to prefer flat hierarchies above nested ones 😉

@carloslancha, the pattern I'm seeing in metal would result in something like: clay-alert/all/alert.js. Could we do that for now here and also update the dropdown case to follow?

Thanks!

carloslancha commented 6 years ago

@jbalsas there you go!