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

Extract components from components/utils into their own folder #324

Open
Mohammer5 opened this issue Apr 13, 2023 · 4 comments
Open

Extract components from components/utils into their own folder #324

Mohammer5 opened this issue Apr 13, 2023 · 4 comments

Comments

@Mohammer5
Copy link
Collaborator

Mohammer5 commented Apr 13, 2023

Relates to DHIS2-15080

components/utils folder

src/components/utils should have a better name. I'd argue for splitting it up into the components that are inside. src/components is already an "abstraction" (components that are shared), adding another architectural layer of abstraction makes the architecture more complex & complicated. It's also debatable which component is a util and which is not, which introduces ambiguity. With my suggestion to centralize/simplify the state in the sidebar (see one headline above) and subsequently being able to remove HidePreventUnmount, we can get rid of this folder as there's nothing in there right now

@Mohammer5
Copy link
Collaborator Author

By @Birkbjo:

src/components/utils should have a better name. I'd argue for splitting it up into the components that are inside. src/components is already an "abstraction" (components that are shared), adding another architectural layer of abstraction makes the architecture more complex & complicated. It's also debatable which component is a util and which is not, which introduces ambiguity. With my suggestion to centralize/simplify the state in the sidebar (see one headline above) and subsequently being able to remove HidePreventUnmount, we can get rid of this folder as there's nothing in there right now

I'm not sure that utils and src/components would be the same abstraction. It's for utility components. I think it's perfectly valid to have a folder like that, and we will most likely end up with components that fit that, instead of creating separate folders for small utility components like these.

We can probably do without HidePreventUnmount, but I do think it solves a valid problem and actually simplifies the state handling in this case. But if we don't want to keep the previous state after clearing the filter (as @cooper-joe seems to suggest), it's not needed for now.

@Mohammer5
Copy link
Collaborator Author

I'm not sure that utils and src/components would be the same abstraction. It's for utility components.

That's not what I was trying to say, but that having two nested abstraction-folders is increasing complexity.

I think it's perfectly valid to have a folder like that, and we will most likely end up with components that fit that, instead of creating separate folders for small utility components like these.

Another point I made is that it can be debatable which component is a util and which is not, which introduces ambiguity. I think HidePreventUnmount is a very good example for what I'm trying to point out. As a developer, I know that src/components will contain shared components, the abstraction level is quite clear. Then inside that folder, there's the utils folder which I wouldn't know what to find inside conceptually. Why would HidePreventUnmount be a utility? What's the difference between that component and Loader for example, which could be considered a utility component but is not in the utils folder. Just like DefaultLazyLoad.

Considering my comment to move what's inside components/card & components/overview into pages/overview (as that is the only place where these will be used, they're not really that "shared"), everything that'd be left in components could be considered a utility component.

So having utils seems very ambiguous to me and therefore also reduce discoverability due to the extra level of nesting. I can't see the advantage or necessity of that folder yet, but on the other hand I can see disadvantages and lack of clarity.

We can probably do without HidePreventUnmount, but I do think it solves a valid problem and actually simplifies the state handling in this case. But if we don't want to keep the previous state after clearing the filter (as @cooper-joe seems to suggest), it's not needed for now.

I can't see the component being used anywhere right now.

@Mohammer5
Copy link
Collaborator Author

By @Birkbjo:

Another point I made is that it can be debatable which component is a util and which is not, which introduces ambiguity.

Why would HidePreventUnmount be a utility? What's the difference between that component and Loader for example, which could be considered a utility component but is not in the utils folder. Just like DefaultLazyLoad.

Yeah I agree that it can be a bit arbitrary. I'm not sure if it's ambiguous though - it's just a decision where it lives. And everything we do are decisions right? And can look arbitrary if you don't know the context or reason behind a decision. But that doesn't mean it's ambiguous. utils is just a catch-all for small utility components that doesn't "deserve" it's own folder. I do think both Loader and DefaultLazyLoad could be in a utils-folder, but as they have styles attached, and thus not in one file I decided to move it to their own folders. I think introducing a folder for just one file is a bit overkill, and I wouldn't even be sure what to call the concept of HidePreventUnmount - it's just a small utility component.

@Mohammer5
Copy link
Collaborator Author

Mohammer5 commented May 1, 2023

@Birkbjo

I'm not sure if it's ambiguous though - it's just a decision where it lives.

It is ambiguous until there's a clear rule about what lives where. Whether a component is "complex" or "just a utility" is subjective by nature, so until there's a clear rule, I don't see why this wouldn't be ambiguous.

I think introducing a folder for just one file is a bit overkill, and I wouldn't even be sure what to call the concept of HidePreventUnmount - it's just a small utility component.

I don't think that a folder would be overkill; it's not much work, it keeps things consistent and simple. One alternative I could live with is to have components that don't have any co-located files directly in src/components. That's a straightforward rule, there's no ambiguity, and it keeps things simple.

To me, this just seems to be an overabstraction architecture-wise. I don't see much value in having these components nested in an extra folder; it seems to be an anticipation to solve a potential issue that is not there yet. Thanks to using index files as entry files for our modules (folders), we don't have to worry about the internal architecture from the rest of the app's perspective. So it makes sense to keep things as simple as possible and only introduce more complex patterns when needed, not before.

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

No branches or pull requests

1 participant