refactor hardcoded layout strings

[adriangohjw]

Problem

Currently all layout are hardcoded as string throughout the app, across UI and Studio.

This makes it hard to debug/improve + more prone to error due to carelessness

Solution

Breaking Changes

• Yes - this PR contains breaking changes
  • Details ...
• No - this PR is backwards compatible

Features:

• Details ...

Improvements:

Using a Single Source of Truth approach

• import ISOMER_PAGE_LAYOUTS and use it whenever possible instead
• create and import TAILWIND_SIMPLIFIED_LAYOUTS and use it whenever possible instead

[vercel]

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Comments	Updated (UTC)
isomer-studio	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Nov 11, 2024 2:42am

[datadog-opengovsg]

Datadog Report

Branch report: adriangohjw/refactor-hardcoded-layout-strings
Commit report: d06b8ff
Test service: isomer-studio

✅ 0 Failed, 167 Passed, 34 Skipped, 35.4s Total Time
➡️ Test Sessions change in coverage: 1 no change

[dcshzj]

This makes it hard to debug/improve + more prone to error due to carelessness

I'm a bit confused by this statement. I feel we should lean in to the inferred types from TypeScript, which can provide an early warning system specifically for such issues. The layout prop is inferred by TypeScript which allows us to specify things like "content" safely, as "conteent" would not match the allowed type and throw an error. This also allows us to only define the universe of layouts in a single place, instead of having potentially two sources of truth.

If there are places where this wasn't caught, we should fix the root cause which is that the types are not being defined correctly.

[adriangohjw]

You have a point!

Before starteing this PR, I reflected on the challenges I had in the past weeks, esp. as a newcomer to the codebase. There's some area of improvements that slowed down my onboarding which I hope I can address to make things easier for others in the future.

The goal of this PR is to improve two key areas:

1. Ease of maintenance – Making future changes more consistent and ensuring the same values are used throughout the codebase.
2. Development speed – Especially for newcomers, reducing friction and confusion by providing more clarity and context.

Yes, we could address this with more documentation, but IMO documentation can sometimes cause more problems than it solves. This is esp. true if it isn’t regularly maintained, which will happen to us more often since the team is limited in time and manpower (also, keeping it up-to-date is more chore than anything)

Here are a few examples to illustrate this (Note: they are overlapping in some cases)

• Searchability: Instead of hunting for a string like "content", it's much faster to search for a constant like ISOMER_PAGE_LAYOUTS.Content. This is especially useful when strings like "content" are ambiguous or reused in different contexts (e.g., as a link or file in Button components).
• Clarity for newcomers: Sometimes, we have overlapping terms that might be confusing.
  • For instance, the term homepage could refer to either a layout or a Tailwind simplified layout. By using constants like ISOMER_PAGE_LAYOUTS.Homepage or TAILWIND_SIMPLIFIED_LAYOUTS.Homepage, we can eliminate this ambiguity.
• Explicitness: In cases where values are used as keys (not just as strings), it’s clearer to write [TAILWIND_SIMPLIFIED_LAYOUTS.Default]: {...} instead of default: {...}. This makes it more explicit and easier to understand at a glance.

To be clear, this change will probably have minimal impact when it comes to enforcing type checks. Since both ISOMER_PAGE_LAYOUTS.Homepage and "homepage" are ultimately the same string value, it will unlikely trigger any new errors that didn’t already exist.

[dcshzj]

nit: for standardisation should we also use ISOMER_PAGE_LAYOUTS here?

[adriangohjw]

yes but... i have no idea why npm run build fails with a totally unrelated error message when i import that. Spend quite some time trying to debug that but couldn't figure out why too

[mergify]

This pull request has been stale for more than 30 days! Could someone please take a look at it @opengovsg/isomer-engineers

[mergify]

This pull request has been stale for more than 30 days! Could someone please take a look at it @opengovsg/isomer-engineers

[adriangohjw]

too stale - will create it in another PR subsequently