Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: update decomposition cheatsheet #697

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

falkomerr
Copy link
Contributor

No description provided.

Copy link

netlify bot commented Jul 24, 2024

Deploy Preview for pr-fsd canceled.

Name Link
🔨 Latest commit caaeddc
🔍 Latest deploy log https://app.netlify.com/sites/pr-fsd/deploys/66a0c28c3f2e0b000815ed0b

@illright
Copy link
Member

Is there any particular reason that you want this cheatsheet back? I'm hesitant to bring it back even with a disclaimer of some kind because I don't think it's useful in its current form, and it can be rather confusing. Our problem right now is that we have too many definitions that are too narrow, we need to widen the definitions, not constrain them even further

@falkomerr
Copy link
Contributor Author

falkomerr commented Jul 25, 2024

this cheatsheet really simplifies both the decomposition process for beginners and the overall understanding of what these layers should do

Copy link
Member

@illright illright left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I left a few comments regarding the writing, but I still don't want to bring back that decomposition cheatsheet. It propagates the idea that widgets and pages are just combinations of the layers below, an idea that ended up being quite toxic to the codebase and people's understanding of FSD. There are still some other parts of the docs that propagate the same idea, but I'm working on rewriting them. As for the cheatsheet, I would only be willing to merge this once I rewrite the cheatsheet itself a little bit. I'm not sure when I'll get around to this, but I'll try not to make it too long

---

# Decomposition cheatsheet

Use this as a quick reference when you're deciding how to decompose your UI. PDF versions are also available below, so you can print it out and keep one under your pillow.

## Choosing a layer
## Decomposition by necessity {#decomposition-by-necessity}
It is a common mistake to assign separate slides to the part of the code that is not reused anywhere.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

issue: typo, and also the phrasing is a bit unclear

suggestion:

Suggested change
It is a common mistake to assign separate slides to the part of the code that is not reused anywhere.
It is a common mistake to extract a certain part of code into a separate slice when it's not reused anywhere.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

also, it's not always a mistake to extract code into a slice when it's not reused. Sometimes, large chunks of code can be extracted into layers below to decrease the size of a page, it's just that these decisions need to be taken consciously and only when the size of the page is a problem (usually it isn't)

This mistake creates several problems at once:
- The codebase becomes more disjointed, making the code harder to navigate.
- A person reading your code, seeing that a slice is not part of the page, may begin to question whether they are breaking part of the display by changing that slice.
- This is a violation of basic principles such as [KISS](https://en.wikipedia.org/wiki/KISS_principle) and [Occam's Razor](https://en.wikipedia.org/wiki/Occam%27s_razor).
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

suggestion (non-blocking): I'd say it's more of a violation of YAGNI (you ain't gonna need it)

It is a common mistake to assign separate slides to the part of the code that is not reused anywhere.
This mistake creates several problems at once:
- The codebase becomes more disjointed, making the code harder to navigate.
- A person reading your code, seeing that a slice is not part of the page, may begin to question whether they are breaking part of the display by changing that slice.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

issue: it's not clear to me what "display" is

suggestion:

Suggested change
- A person reading your code, seeing that a slice is not part of the page, may begin to question whether they are breaking part of the display by changing that slice.
- A person reading your code, seeing that a slice is not part of the page, may begin to question whether this code is used somewhere else, and if they are breaking anything by changing it.


For example, we have a component that contains a block of information about a user. This component is only used on the profile page, so it should stay on the profile page.

If you are sure that your slice will be used in more than one place, you can use the note below.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

suggestion:

Suggested change
If you are sure that your slice will be used in more than one place, you can use the note below.
If you are sure that your slice will be used in more than one place, or you have another very good reason to decompose the code into the lower layers, you can use the note below.

@falkomerr falkomerr marked this pull request as draft August 14, 2024 14:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants