RFC: Reusable components

patrinoua commented 2 years ago

Proposal

mobile dev specific rfc

Create components in a reusable way.
Small and medium size components (atoms, molecules, organisms) are developed in a dedicated folder (palette) where they can be easily discovered, and reused by other engineers. Moreover it will be easy to transfer them between different projects (eg. eigen, energy)
Use the same structure as in Design (atoms, molecules, organisms)
Components are always created on a common place instead of locally in the files where they will be used

Reasoning

Modularity

Simple things make firm foundations https://www.w3.org/DesignIssues/Modularity.html

One module can evolve or be replaced without affecting the others. If the interfaces are clean, and there are no side effects, then a developer can redesign a module without having to deeply understand the neighboring modules.
easy to update
easy to debug
easy to test
easy to demonstrate using storybook
everyone knows where components are and they don't have to redevelop

React components are made to be reusable

Components allow you to split the UI into independent, reusable pieces, and think about each piece in isolation.

According to the react docs https://reactjs.org/docs/components-and-props.html

What are the advantages of reusing components?

Code that is less prone to bugs (we only need to debug in one place instead of 3 for example)
Speed (we only need to create things once) and with a quick look on storybook we can know if something has already been created (and tested) or not.

Additional Context

Atomic Design is a way to organise our components library with a hierarchy depending on size (atoms are one piece, molecules are a combination of atoms and organisms are a combination of molecules).

Read more about atomic design here https://atomicdesign.bradfrost.com/chapter-2/

How is this RFC resolved?

We decided to close this RFC with out resolving it. A new RFC will follow.

dzucconi commented 2 years ago

Is Palette not reusable/discoverable? https://palette.artsy.net/ + https://palette-storybook.artsy.net/ — or is this just concerning iOS related work?

patrinoua commented 2 years ago

In eigen and now energy it often comes up that we develop components locally, this is what this RFC is addressing. Here is an example.

dzucconi commented 2 years ago

Ah, I see; I'd then alter this RFC to specifically concern mobile development as this is normal practice for web. (Y'all should just extract a palette-mobile design system package.)

patrinoua commented 2 years ago

Thanks, I've added that in the description :)

MounirDhahri commented 2 years ago

@dzucconi to your point, we do use palette as well in mobile and as far as I know, we have as well the atoms created by design extracted inside it.

MounirDhahri commented 2 years ago

@patrinoua IMO, I believe we should still stick to the same logic described here and that we've been using so far

Does my component belong in Palette?

If the component applies to Artsy as a brand and can/will be used across multiple digital products, then Palette is a great place for it. If it's highly product specific then it's best to leave the component where it's used. We can always move things later!

The reason behind that is that we still want to keep using palette (or palette-mobile) for both eigen and energy. Both repos could still have their own shared components, and that's totally fine IMO, but I don't think those components should move to palette unless they get added to our design system by design. The example you cited above (this one), is a good example of a component that's only specific to energy that I don't think we should be moving to palette

patrinoua commented 2 years ago

@MounirDhahri thanks for elaborating.

My suggestion is to have components developed in a central place of information.

Advantages being that 1) It does not take extra time 2) the file can be in one central location with its test and storybook file (clean and modular code) 3) If/when we need to use it again we can quickly find it

The equivalent in a kitchen would be keeping the spices with spices, pasta with pasta and cutlery with cutlery instead of having everything in one shelf. 👩🏻‍🍳

We have a figma file with the design system (currently V3.1) and we can assume that we use the same design system through out the app/apps, but if the different apps have different design systems we can easily create a palette for each design system. That could be palette-mobile, palette-eigen or palette-energy or whatever the scope of it. However, since I don't think there would be so many differences between the components I do not see a need for that diversification.

Even if a component is only going to be used once by one app, it's still good to have it on a central place instead of inside the place where it will be used. Because if it is used once, who knows, maybe we'll need it again, and then it will be ready for us.

Otherwise we are assuming that when they want to create something that already exists developers know the app so well and know where they can find things that are locally defined. Then because of spaghetti code there is the risk of having to un-spaghetti the code before reusing it, and then go ahead and copy paste code locally.

This increases friction when we want to update or debug a component, because maybe the component is updated in one place but not everywhere.

It often happens that people develop a component that already exists from scratch because they didn't know the component is already developed somewhere and it's easier to just make it from scratch than search around a huge code base.

The opposite of what I am suggesting is defining things locally in the file where they are used, which is often the case now. I do not see the advantage of doing things this way, however if there are some I'm interested in finding out more about that.

damassi commented 2 years ago

@patrinoua - can we concretely show show how it currently is, and how you would like it to be, maybe with a mock file tree and some code examples in the description? I'm having a little trouble understanding. Is the RFC a request to update organization of existing palette components around atomic design?

I see this comment here, and we should certainly be more diligent about creating reusable components. If indeed the ArtistThumbnail here is generic enough to be reused globally, why not move it to a centralized location? Often times theres a balance tho -- there's something that should be generic, but in practice its not worth it and a one-off component is fine.

pvinis commented 2 years ago

I'm kinda in the same boat as @damassi.

if this is about:

implementing atomic? yep, im in. should we do all at once, or as we go? I think an as-we-go approach might be easier.
moving all react components to palette-mobile? I don't agree. A lot of components that are there don't get used enough, and a lot that should be there aren't. Again, an as-we-go approach might be better. we can start from local and if we end up reusing, we move to palette-mobile. or we can start from palette-mobile but be very mindful that maybe this new component if very specific to what we are doing, so we can choose to not start by default in palette-mobile.
moving some react components to palette-mobile? then which ones? like chris said, i would love a before and after, and we can compare and discuss if that makes sense, so we can come up with some rules for when something goes to palette-mobile or to local. also, what about hooks, utils, helpers? do we treat them same as the components or not?
something else? im confused then 😅.

I feel like we all agree with your bullets and the search results and all that. Maybe some of us don't apply everything all the time, and that's ok, if it's not disrupting the rest of us. These "principals" are there as defaults, not as rules. Show me one developer that has never deviated from DRY, and I'll show you a bad developer, pointing to that same person. So what I'm getting at is, what do we want this rfc to do concretely? Instead of agreeing or disagreeing to this rfc as it is now, and then have the action items be determined afterwards, I would like to see very specific changes that you would suggest, and we can agree or disagree on these.

araujobarret commented 2 years ago

I'm also a bit confused 😅 ; we already do that in Force and Palette and we thrive for this type of modularity, sometimes re-visiting components to remove some extra batteries and/or add.

In regards to the way we design the components @damassi made it simple, IMO this RFC needs some examples of good/bad cases.

patrinoua commented 2 years ago

I am sorry if this is confusing, I will try to elaborate more on my suggestion.

Jonathan and the rest of the designers took the time to organize the different components we use in atomic design principles.

I suggest using the same principles to organize our components with code.

This way we won't have to debate on each component "Should I add this to palette, should I not add it to palette"

Adding things to palette is free ✅ and it makes them easier to find ✅.

So I am wondering why we would not add things there (in our toolkit) and define them locally instead.

I am suggesting a priori rather than a posteriori approach, where we expect that components might be reused and have them ready, rather than define them once locally and the second time we want to use them either define them from scratch (the quick solution) or search for them, modularize them and add them to palette.

patrinoua commented 2 years ago

Here is also an example:

We have at least 3 places on eigen where we have upload photo functionality.

Because we do not have a modular component for doing that job, last time we had to do that we had to create the functionality from scratch, with copy paste code. Here is the PR.

Photo upload is a process much more husty and we dedicated at least a week last time we developed it, with using all the functions with all the repos, testing etc.

If we had a generic component we could easily just use that one and not have to dedicate all that time on making it from scratch which is basically copy paste and change a few things.

And then next time the developer would have to do the same: The whole process again, copy paste and change a few things.

Imagine now we have this process defined 4 times, with a few differences at a time. Something changes so the API works a bit differently now. So now we have to refactor this in 4 places instead of just one.

patrinoua commented 2 years ago

It's not about thinking for each component "should I make it reusable?" but about having the mentality, that if I make it once, I better make it reusable, because who knows I might need it again in the future.

I personally don't think it requires that much effort to do that which is a big part of why I am bringing this up as a conversation.

pvinis commented 2 years ago

not have a modular component for doing that job, last time we had to do that we had to create the functionality from scratch, with copy paste code. Here is the PR.

One could argue that this could be the point where this component becomes shared. Maybe not on the first use, maybe not the second, but maybe the third we move it over. 🤔

patrinoua commented 2 years ago

Well maybe we dont need to use it again, who knows? What is the advantage of doing it on the third time after I dedicated 1 week every time instead of just doing it from the first time? I'd like to understand the blocker. We don't want to overcrowd our palette with elements we may just use once? Why? Is it bad for performance, too much info or what is the argument against doing that?

pvinis commented 2 years ago

Well maybe we dont need to use it again, who knows?

that goes against what you are suggesting? You said we should go to palette-mobile by default, but here, if we don't use it again, we shouldn't?

what is the argument against doing that?

others might feel differently, but for me personally, I guess I try to judge if something is going to be reused or not. Not always a correct guess. But I feel that first guess becomes more concrete after something I thought would not be reused, ends up actually being reused. and I prefer to start local, only because (to me) if feels easier to "promote" something from local to palette-mobile, than it is to "demote" from palette-mobile to local. I'm not saying that way is correct, or the best way. I'm just saying that's usually on my mind when I need to decide where to put a component.

But I agree that many times we don't go to palette-mobile even when we should. Sometimes it slips my mind. Sometimes I'm biased by the workflow that I don't want to do the 1, make change on palette, 2, make a pr, 3, wait for canary, 4, try it on force. That's why I want us to do better for palette-mobile. And maybe an easier/faster workflow would make me and others be more easily convinced to put things in palette-mobile directly 🤔

patrinoua commented 2 years ago

Just playing devils advocate! 😈

I also think that once you've already used the component a few times (3 in this case) maybe you don't need it a 4th time.

But maybe you do!

If we had made a component reusable in the first place we would have skipped the extra time of developing it for the second and third time and refactoring it 3 times, if we now decide to make it reusable.

I do not believe in a class system where some components deserve to be in palette and some not. All components are equal and deserve to be palettised and reusable! Tested. Posing on Storybook. 💃

And yes this should be part our normal development circle and not an extra thing to do that needs too much work, otherwise it would never work :)

dzucconi commented 2 years ago

All components aren't equal though.

There's:

Broadly reusable atoms which are generic design system components that belong in Palette — an external package that can be reused in different applications.
Reusable molecules that are unique to the application but can be re-used within the specific application. (These frequently include associated data fragments).
One-off snowflakes specific to a screen.

There should be an effort to locate and extract common components within an application, of course. Recently in Force we located and extracted common gridded cells as well as associated entity stubs: https://github.com/artsy/force/tree/main/src/v2/Components/Cells + https://github.com/artsy/force/tree/main/src/v2/Components/EntityHeaders — these things are application specific and include fragments for their reuse. They wouldn't be appropriate to put in Palette: we don't want anything in Palette to have fragments associated with them and there are no other apps that would use them. Adding them would increase complexity, package file size, and sacrifice maintainability.

There is also a cognitive cost to having every single component in a flat hierarchy. It makes more sense to me to just encourage extraction in code review if you notice a common pattern or someone re-inventing the wheel. (Part of that is also locating similar patterns and encouraging the design team to iron out any inconsistencies, which will frequently impede reusability or lead to props API messes if unchecked.)

patrinoua commented 2 years ago

Thanks @dzucconi.

I agree that the package size could be bigger. This is why we could have different palette scopes (eg palette-eigen, palette-energy). We could then easily copy things from one or the other whenever we need to use something.

This would be more performant in comparison with redefining the same component again and again, and easier to maintain, update, and debug.

To your second point about flat hierarchy, I am not suggesting a flat hierarchy, but rather adopting the same hierarchy we use on our design which is following the atomic design principles. You can find out more about it here: https://www.notion.so/Atomic-Design-2ab2d50063934e87b5f989c0a8c31b64

I hope that makes sense 🙏

damassi commented 2 years ago

It's worth noting that there are also different dev responsibility levels within an application. Every-day product devs aren't necessarily responsible for understanding the bigger picture. For that there's specialization: higher-level devs notice patterns and are generally responsible for gathering these things together and refactoring, typically all at once (see for example @dzucconi 's work around EntityHeaders). Putting this big-picture responsibility on product devs can lead to more disorder, not less, as well as less efficient product cycles.

patrinoua commented 2 years ago

Thank you everyone for your contributions. 🙏 After collecting feedback both from here and from 1-1 conversations, I'll now close this RFC and soon make another one, where it is more clear what we are hoping to achieve and in which way this could be done.

artsy / README