fix: create and organise CheckList page folder with dependent components

[ckbedwell]

Part 2 of addressing: #848

I plan on breaking the above ticket into separate PRs. Because of the nature of the work it is difficult to address up front how many there will be but because each PR will touch many files doing mundane things I intend to silo the work as much as possible to make review and discussion more relevant.

Note: Targets part 1 to reduce noise.

Problem

As a developer I would expect if I were to read the src/routing/InitialisedRouter.tsx file that each Route / Page I saw would have its own logical entry in the page folder. At the moment it is a messy mixture of components being sourced from the src/components folder and src/page folder.

This particular PR addresses moving the CheckList 'page' to its own folder.

Solution

• Moving CheckList to its own folder.
• The folder has its own generic files / folders
  • components
  • hooks
  • constants.ts
  • types.ts
  • utils.ts

☝️ The above is the intended convention which will be adopted by all of the pages I intend to look at in the following PRs, so it is important to try and get this right now.

The intention is that all of the above which is only used on the CheckList route is co-located to the corresponding file. As soon as something needs to be used elsewhere outside of the CheckList page it would 'graduate' to a more generic area as appropriate.

Naturally, there are pros and cons to this approach but the positives outweigh the negatives. The main one to note is acknowledging it becomes more difficult to discover components which should perhaps, or be expected to, be generic which are now 'buried' in a page's folder.

A good example in this PR is the Pill component. It is only used on the CheckList page and specifically only within the CheckListItem. I made this decision because I want it to be clear when refactoring and modernising the repo as we move forward the extent of what would be affected in removing / updating / refactoring certain components.

It adds a burden on our developers to 'know' or be willing to search within page folders to see if something similar exists before re-creating their own implementation and assessing whether that file / component can be reused but should result in higher quality and more considered components rather than adding the expectation on developers creating generic components being able to predict the future in what would be the ideal abstraction for multiple use-cases across the application.

Questions

Should we rename the page folder to something else or happy with how it is? (pages or routes?)

[w1kman]

I love the approach of not scattering files all over the place and instead keeping them in context. <3

I want to challenge the depth of the directory tree a little.
I don't think having an excessive folder structure within the "context" adds value (it feels like two steps forward and one and a half steps backward).

Example of excess (especially for a maintainer):

page/
  CoolPage/
    index.ts
    types.ts
    utils.ts
    constants.ts
    CoolPage.tsx
    CoolPage.test.tsx
    hooks/
      useMyHook.ts
      useMyArm.ts
    components/
      CoolItem/
        index.ts
        CoolItem.tsx
        CoolItemKey.tsx
        CoolItemValue.tsx
      ...

What I would like (better dev experience both as an author and as a maintainer):

page/
  CoolPage/
    index.ts
    types.ts
    utils.ts
    hooks.ts
    constants.ts
    CoolPage.tsx
    CoolPage.test.tsx
    CoolItem.tsx
    CoolItemKey.tsx
    CoolItemValue.tsx
    ...

What I would really like (file-nesting):

page/
  CoolPage/
    index.ts
    CoolPage.tsx
    CoolPage.types.ts
    CoolPage.utils.ts
    CoolPage.hooks.ts
    CoolPage.constants.ts
    CoolPage.test.ts
    CoolItem.tsx
    CoolItemKey.tsx
    CoolItemValue.tsx
    ...

// How it would look in the IDE (with file nesting)
page/
  CoolPage/
    index.ts
    CoolPage.tsx
    CoolItem.tsx
    CoolItemKey.tsx
    CoolItemValue.tsx
    ...

Directory should not have PascalCase
src/page/CheckList/components/Thresholds => src/page/CheckList/components/thresholds
Why: There is no Thresholds.tsx

[ckbedwell]

I love the approach of not scattering files all over the place and instead keeping them in context. <3
❤️

I want to challenge the depth of the directory tree a little. I don't think having an excessive folder structure within the "context" adds value (it feels like two steps forward and one and a half steps backward).

I think this is a really good call out and love the suggestion to explore the file-nesting. Going to do that now!

Directory should not have PascalCase

Good spot! 👍

[w1kman]

Couldn't not whine, but it's LGTM

[VikaCep]

I noticed we have a CheckList/components/ folder where several components used on this page are stored, and a CheckList.hooks.ts file which currently contains only the useCheckFilters hook.

If we ended up adding more hooks and the file were to grow too long (which could make it harder to navigate for developers), what would be the intended approach? Should we continue to keep all hooks in the single CheckList.hooks.ts file in that scenario, or would it be better to create a CheckList/hooks/ folder then and organize the hooks there? I'd just try to avoid creating super long files that might be difficult to read/manage.

[VikaCep]

I really like these changes, I think they’re a great improvement!

That said, I'm not entirely convinced about the following scenario. When creating components, even if they’re only used in one place initially, there are cases where they might be inherently more generic. For example, the SearchFilter component (which provides an input field with a clear button when it contains text) is currently only used on the Probes selection screen but could easily be reused elsewhere in the future. In these cases, I’m not sure it makes sense to store it in a deeply nested folder, especially if it has the potential for broader use across the app. Would it be wrong to store it in a higher-level folder in such situations?

[VikaCep]

I'm not seeing the green color:

The red works fine.

[w1kman]

I really like these changes, I think they’re a great improvement!

That said, I'm not entirely convinced about the following scenario. When creating components, even if they’re only used in one place initially, there are cases where they might be inherently more generic. For example, the SearchFilter component (which provides an input field with a clear button when it contains text) is currently only used on the Probes selection screen but could easily be reused elsewhere in the future. In these cases, I’m not sure it makes sense to store it in a deeply nested folder, especially if it has the potential for broader use across the app. Would it be wrong to store it in a higher-level folder in such situations?

If an export is reused outside the main component directory, it should be lifted out of that directory as it no longer belongs to a particular parent. In some cases, it will become copy/paste, in other cases you may want/need to create a new modified component that fits both/several use cases.

The benefit is that you gain a sense of what is going on, just by looking at the file structure.

The way it has been is that anything that is a component lives in components/. Ideally, components inside the component/ directory are meant to be reusable and not niched to fit only a single use-case, often requiring a lot from the DOM Tree to even present well.

[w1kman]

I noticed we have a CheckList/components/ folder where several components used on this page are stored, and a CheckList.hooks.ts file which currently contains only the useCheckFilters hook.

If we ended up adding more hooks and the file were to grow too long (which could make it harder to navigate for developers), what would be the intended approach? Should we continue to keep all hooks in the single CheckList.hooks.ts file in that scenario, or would it be better to create a CheckList/hooks/ folder then and organize the hooks there?

It's highly unlikely that we'd end up with one Component that would consume a large number of hooks.

Let's say that we'd have one Component.hooks.ts that happens to consist of a few hooks but a large number of lines. Most modern and unmodern IDEs have support for collapsing scopes, which would make it as easy to navigate as looking at file names in a directory. Many IDEs also have directory like navigation of functions/variables that offer the same kind of "navigation" possibilities as a directory with a bunch of files does (+ you'd see the hook signature and return type).

It's when you have a couple of hooks that have their own (non-exported) helper functions inside the Component.hooks.ts file it gets a bit crowded (That is when you create a Component.utils.ts file 😅 ).

A bonus (with a IDE that supports it) with Component.hooks.ts is that you'd be reminded to remove unused exports/code. Something (at least me IDE) don't do with file names

I'd just try to avoid creating super long files that might be difficult to read/manage.

With file nesting, Component.tsx would act as a directory for Component.hooks.ts, meaning that they'd be the opposite of hard to read/manage, as they are grouped and not "mixed" with other files.

The base branch was changed.

[ckbedwell]

When creating components, even if they’re only used in one place initially, there are cases where they might be inherently more generic.

I understand your concern but my issue is that it creates a relatively large burden on the developer to predict the future to create a neatly abstracted component for use-cases that haven't come up yet.

This isn't to say you should be creating components without future use-cases in mind but if you get it wrong or overlooked something you aren't creating unnecessary tech debt and can course correct easier when lifting the component out of its page folder and into the generic components folder.

There is a trade-off with this approach and it does mean the developers have to 'know' the repo better to identify when page-specific components should be lifted out of their specific use case and made generic so they can be used more broadly -- but honestly that's more of a feature than a bug with this approach as it increases our collective knowledge of what we are working with.

[VikaCep]

LGTM! Let's try it out 👍