search

rsm-hcd / AndcultureCode.JavaScript.React.Components

Common atomically designed react components used at andculture
6 stars 8 forks source link

Standardize Atom & Molecule classification #84

Open alexgetty opened 3 years ago

alexgetty commented 3 years ago

As defined in the docs, Atoms are supposed to be a single "component" (I'm assuming this means HTML tag). This also implies that an Atom cannot contain another Atom, and any component that does contain an Atom is at least a Molecule.

There are numerous components currently classified as Atoms that, by this definition, are actually Molecules. I propose moving them to the Molecules layer.

atoms/*/anchor-with-icon.tsx => span element. Anchor and Icon components. atoms/*/checkbox-button.tsx => label, span, and input elements. Icon component. atoms/*/checkbox-input.tsx => label, input, and span elements atoms/*/radio-button-input.tsx => div, input, and label elements atoms/*/select.tsx => div, and select elements. Icon component. atoms/*/text-input-icon.tsx => div element. Icon and TextInput components. atoms/*/progress-bar.tsx => 2 div elements. atoms/*/toast-templates.tsx => 3 div elements. Icon component. atoms/*/heading-icon.tsx => div element. Icon and Heading components.

alexgetty commented 3 years ago

One exception I could see with this is the addition of an icon to an inline element like a link, button, etc. In this context, icons act as if they are characters and it may be easier to have components that can support icons without having a separate version of the component in the Molecules layer.

wintondeshong commented 3 years ago

@TheDataDesigner I agree with the recommendation to better align these components (listed above ☝️) with atomic design principles. Additionally, I can get behind the exception of icons. It is ideal not needing to spin up a parallel component just to add a visual element.

What do others think? (CC @Stefanie899 @brandongregoryscott ...)

Stefanie899 commented 3 years ago

@TheDataDesigner @wintondeshong This change makes sense to me, all the items described sound like good candidates to refactor and ensure we're following atomic design.

I'm on board with allowing icons as an exception, it feels like it would be overkill to be creating molecules just to add an icon to an atom.

alexgetty commented 3 years ago

@wintondeshong @Stefanie899 thanks for the feedback. I can work on this today. There is a related issue (#87) that may make sense to work on together. That issue proposes merging any component with -icon in the name into its base component. How do we feel about doing these two issues together?

wintondeshong commented 3 years ago

@wintondeshong @Stefanie899 thanks for the feedback. I can work on this today. There is a related issue (#87) that may make sense to work on together. That issue proposes merging any component with -icon in the name into its base component. How do we feel about doing these two issues together?

Yeah @TheDataDesigner they almost make sense to be merged into one issue at this point as well as the effort 👍

alexgetty commented 3 years ago

Here is the message from #87:

There are a few components that have a separate version of the same component with the addition of an icon. I propose that those get merged with the base component so that it has an optional icon property that can be passed in. If it isn't null, an icon is rendered. This would help standardize components so the same patterns can be found across components, simplify the component search/selection process, and make our components more customizable and robust.

anchor-with-icon.tsx would be merged into anchor.tsx button.tsx would be extended to include an icon prop instead of inserting an Icon component as a child text-input-icon.tsx would be merged into text-input.tsx heading-icon.tsx would be merged into heading.tsx

alexgetty commented 3 years ago

@wintondeshong All of these changes are done and ready in a branch I created from the branch Phil created for #48. Once that PR is accepted and merged, I'll throw up a PR for these changes. I don't want to backlog too many PRs that are dependent on the ones before it.

brandongregoryscott commented 3 years ago

This all makes sense to me 👍 The frontend is always the first place where the layers/patterns start to diverge so wrangling it earlier than later would be good. Echoing @wintondeshong's sentiments about enforcing it with linter/compiler rules too (#88) - that would be a great step toward consistency

brandongregoryscott commented 3 years ago

@alexgetty @phess101 Is there any work left to be done on this one, or did #102 wrap it up? If so, I'll close it out