fabric-ds / issues

Site for tracking tasks and issues related to Fabric
0 stars 0 forks source link

Question: Do we have a clear definition for when it is appropriate to use a component? #24

Open digitalsadhu opened 2 years ago

digitalsadhu commented 2 years ago

When should we provide a component (react/vue/custom element) vs define a CSS class that can be applied to an element?

A couple questions to frame the discussion:

A few possible criteria for when something should be a component..

1. It needs behaviour

If we need to add JavaScript to the component to give it some behaviour then it would need to be a component.

2. It has a markup structure

If implementing the item in question requires more than a very simple HTML structure, then it's might benefit from encapsulation in a component of some kind.

3. It should be isolated

Only really applies to shadow dom/custom elements and unsure how much this should/would really affect the general decision to implement something as a component instead of CSS class but if there were a need for isolation then we could only get that with shadow Dom so it would have to be a component.

4. It doesn't need to be used in a server rendered only environment

Slightly controversial point perhaps but we have many apps that don't use React or Vue and are not client side only. These apps are left out in the dark with any kind of component for now, even custom elements. These types of apps tend not to contain forms at all so its much easier for them to get away without React/Vue. Good examples are the front-page and the market front-pages. How should these implement box for example?

Thoughts? cc/ @benja @Skadefryd @pearofducks @adidick

pearofducks commented 2 years ago

One bit of missing context: .page-container, form elements, and a few others - stem mainly from when we were trying to make Fabric (nearly) 100% backward compatible with Troika. They're probably poor examples of our modern "why" we would take a certain approach

I'd rather flip this - and say why would we ever want things to be a class? Especially if we already are eating the TW lib, why do we want to bloat CSS every time we add a component (or a feature to a component)?

digitalsadhu commented 2 years ago

Ahh right.

pearofducks commented 2 years ago

Unless we want to do a v2 of Fabric which corrects these things, I think we just live with the existing "mistakes".

There's several ways we could attack this:

pearofducks commented 2 years ago

Here's method 2 and method 3 just for funsies:

https://github.com/pearofducks/fabric-vue-js-templating/blob/master/index.js

digitalsadhu commented 2 years ago

Man, SSR in Vue is a lot easier/nicer than React... (mostly because of JSX I guess)

Skadefryd commented 2 years ago

Whats the pros/cons using method 2 vs Aliasing those same classes to a .box class?

pearofducks commented 2 years ago

Pros:

Cons:

benja commented 2 years ago

It's an interesting discussion and great points made!

Regarding point 1:

A component like Box can easily be a few classes (just how we use TW anyways), unless the box requires JS functionalities like click to reveal content etc. If a component is solely for easier markup, then maybe it makes sense to leave it out of the JS component libraries and rather provide a set of helper classes in @fabric-ds/component-classes?

But there are also the cons of introducing classes to the core Fabric file. However, we don't necessarily have to introduce any new classes if we just use pre-existing TW classes in our component-classes. Styles such as padding, margin, colors, etc are all covered in core css — so reusing these probably wouldn't be an issue for creating a component like box. Classes such as .page-container would, by my above definition, continue to be a class.

benja commented 2 years ago

The other question, if we decide to keep the box a component provided it does not provide any JS functionalities, is whether we only implement it in web components so that it's maintained in one place. Or we could just throw components that provide very little JS logic into Elements only as they can be consumed by any JS lib / framework.

Skadefryd commented 2 years ago

So Tailwind is supposed to be a 'highly customizable utilities library' <- not my words. Adding a few utility classes that gives us some global control surely cannot be the worst Idea?

Skadefryd commented 2 years ago

I Agree that we added some stuff in the 'backwards compatability' - era that is not ideal, like the current setup of the buttons.... but to add some aliased classes like .page-holder, I can't really see that as been a mistake, or even a problem... (?) The scenario where everyone just uses a .box class rather then implementing (And us maintaining) different components for different frameworks, how can that be more difficult? Much easier to write good semantics aswell since a class like that could be set on any element instead of been dictated by the component.

Skadefryd commented 2 years ago

These are not really utility classes, I know :D Rather tiny css only components. In my head it is ok for these because they do not dictate markup, rather they are single element stuff... If a css thingy like this dictates a certain markup structure, that's a different story alltogether. Just my thoughts....

digitalsadhu commented 2 years ago

I think I'm seeing several questions in here we need to answer.

  1. Should we be supporting apps that do not use a framework at all?
  2. If we do, how?

I think personally there are enough apps that are server rendered only with no React/Vue etc that the answer to number 1. should be a straight up yes. (we have 8 such apps on our team alone)

If we agree that we should (and I'm not sure we do agree but I'll start from there 😆), then its a question of how best to do so?

@pearofducks has proposed the following:

  1. While "use Vue" actually looks like a pretty ok solution, I'm not sure its a catch all. Would mean forcing people to use Vue and not sure that's our plan.

  2. Custom elements with SSR might be good enough eventually but it's not at present. Might be ok to have some things client side only but.. feels a bit awkward.

  3. That leaves component classes. This should work pretty nicely for Node apps that can pull in an NPM package and drop the variables into template literals.

Between all 3 of these, perhaps we have enough guns in the arsenal to handle the problem? Maybe?

Having said that, I did some reading on the TW docs site here: https://tailwindcss.com/docs/reusing-styles. It seems that there is a concession being made that sometimes, composing together TW classes into higher level classes is a useful approach. Looking at the way @apply works, this seems like a really nice approach and it leaves me wondering, if all the component classes were just implemented in the CSS using apply, how much bloat are we talking? If not for bloat, it feels to me like a nice approach. It's simpler and It works in more places because there's no extra component classes package to worry about. I might be a bit underknowledged (and therefore missing something) here but I don't see that such an approach would be philosophically at odds with TW (given the docs page above)? I think the only disadvantage that worries me is the potential for bloat but maybe its not that big? Perhaps we could test?

pearofducks commented 2 years ago

Forcing people to use Vue is not my intent - I merely mean that's an acceptable alternative (compared to f.ex JS-templating strings) if component-classes aren't appealing, or there's some other complication.

I have a lot of concerns with us going down this route. Part of the "contract" we've always had with Fabric was that we were eating all the CSS size up front to get all these snazzy utility-classes - then people could build literally anything - and we'd at most have to add like 2-10k over the years to support new things. This breaks that contract pretty hard and starts putting a per-component bloat in the core file - and I genuinely don't understand why we'd do that. If we're failing at supporting something that bad, then IMO we should discuss really changing things around or pivoting how we do CSS.

I've added the .box class here - it adds 15k min'd (I suspect the group utility causing a lot of the size increase there)

Skadefryd commented 2 years ago

15k min'd, thats pretty bad

digitalsadhu commented 2 years ago

Yup I can see that too @pearofducks in any case yea 15kb min is way too bad.