Unify overall file structure and code

[ebma]

Context

In #326 we identified the need for some refactorings because we are having duplicate components with different stylings. Besides that, the code style diverged as the development of the Nabla UI happened in parallel to the development of the core portal features.

We should clean this up and unify the components and structure again.

Guidelines

Functional components

Do we want to declare functional components as function MyCompenent(props: Props) or as const MyComponent = (props:Props) => ()? At the moment we have a mixture of both notations.

Folder structure

The ideal folder structure is roughly as follows:

src
  - assets 
  - components
  - hooks
  - helpers
  - pages
  - services

We should remove the folders models, shared, config, and constants and move the contained files somewhere else. We should add a new folder contexts and move all the useContext components inside.

We need to agree on where to put components. Currently, some components live in src/components and some live in src/pages/xxx. This isn't very clear because it is unclear when and where a component is created.
Two options:

1. We move all the components out of the src/pages/xxx paths and into the components folder. The files in pages should only import components to display them, not define them.
2. We move all the page-specific/non-reusable components into the respective pages directory.

Component file structure

Each component should live in a separate file. (Components that are tightly coupled can live in the same file). Component file names should be in Pascal case. When a component has a related styles.css file or test, it should live in an extra directory as follows:

MyComponent
 - styles.css
 - index.tsx
 - index.test.tsx

If neither a styles.css nor test file is relevant for the component, it doesn't need to live in a separate directory and can just be called MyComponent.tsx.

TODO

• Use the same number input component throughout the app. Done in Improve Nabla swap design #483. To be confirmed whether changes still required
• Refactor code to adhere to guidelines

[ebma]

@Sharqiewicz I started fleshing the description of this ticket out a bit more. Do you have thoughts/suggestions on how we should structure the code, what to refactor/align, etc?

[prayagd]

cc @Sharqiewicz incase you missed the message from Marcel

[ebma]

@TorstenStueber if you have any thoughts/opinions about the guidelines I added to the description please feel free to share them as well.

[TorstenStueber]

@ebma Great proposal.

On the question of whether to define components as proper functions or arrow functions, I have the strong opinion that top level functions in JS should always be functions, otherwise JS gets degraded to C/C++ where the definition order of functions matter. Is there any advantage for using arrow functions instead? Also the official React docs use this style.

I mostly agree with the folder structure. Where would you put config/constants/contracts? What is the purpose of services folder, it's not really clear what entails a service, any kind of logic? I also agree to add a folder for contexts.

For the components I tend more towards option 1, but we might then have a large collection of components in the components folder. Shall we subdivide it further into classes of related components?

The CSS styling seems to be a wild combination of classic CSS files and Tailwind. I think it makes sense to unify this as well and slowly migrate towards tailwind (as this is also the technology we use in the Vortex UI now).

As you already pointed out: when there are no test or CSS files, I strongly prefer that we move components into MyComponent.tsx instead of MyComponent/index.tsx.

[Sharqiewicz]

@ebma @TorstenStueber

Components definition

Arrow functions vs Normal Functions
The key distinction between them is how they handle this. A normal function creates its own this context (lexical scope), while an arrow function does not - instead, it inherits this from its surrounding context. Functional React components don’t rely on this - the same for hoisting so it is irrelevant

So it is the matter of style, I prefer arrow functions as they offer a cleaner and more concise syntax ( when combined with TS ):

const MyComponent: FC<MyComponentProps> = ({ prop1, prop2 }) => { ... }

and if the component has no logic you can directly return the JSX without writing return keyword.

Exporting components

I prefer named exports over default exports. Named exports help eliminate the risk of naming errors. With default exports, you can import a component using any name, which can lead to inconsistencies across the codebase.
With named exports, you import the component using its exact name, excluding the chance of misspellings. And what I love is that the IDE can provide autocompletion and show you exactly what is available to import from a file

[Sharqiewicz]

@ebma I've already put some work into unifying the components between Nabla and Portal, removed duplicate components, refactored the components used by both and created tests for the most important ones. But there are still some components to be processed like: nabla/From, nabla/To and all of the components that are in the /nabla directory. I strongly agree it has to be cleaned up

[Sharqiewicz]

@ebma @TorstenStueber WDYT?

src
  - assets 
  - components
  - hooks
  - pages
  - utils (instead of helpers)
  - services
  - constants (for abi) (somehow connect it with config)
  - store/state - for useContext (and maybe Zustand)

remove /shared /models and move the components and files that are directly under /src to their corresponding directories

[ebma]

If it really does not matter much if we use arrow vs normal functions for functional components, I'm more in @TorstenStueber's boat because I personally prefer giving more space to components. Declaring them with a simple const somehow makes them look less important to me. This is obviously very opinionated and may be old school but to me, the perfect component declaration would look like this:

interface Props {
  param: string;
}

function MyComponent(props: Props) {
   const { param } = props;
   return (
      <div></div>
   );
}

I know that we can save some lines by already destructuring the props in the function definition but I prefer having it more explicit. Not too hard feelings about this though.

I prefer named exports over default exports. Named exports help eliminate the risk of naming errors. With default exports, you can import a component using any name, which can lead to inconsistencies across the codebase.

Works for me. I'm fine if we agree to only use named exports going forward.

Regarding the structure, your proposal looks good to me @Sharqiewicz but I would opt for state over store as name for the last directory.

[ebma]

The CSS styling seems to be a wild combination of classic CSS files and Tailwind. I think it makes sense to unify this as well and slowly migrate towards tailwind (as this is also the technology we use in the Vortex UI now).

I would also prefer not to introduce any new .css declarations if possible and only relying on tailwind. However, I think some things can only be declared in CSS unfortunately.

For the components I tend more towards option 1, but we might then have a large collection of components in the components folder. Shall we subdivide it further into classes of related components?

What exactly do you mean with subdividing? Let's maybe talk about examples. I understand that you would group components like the SwapAssetsButton and ModalClose button in a Button directory, similar to the TickerSwapTable and Table? Or are you referring to something else?

What is the purpose of services folder, it's not really clear what entails a service, any kind of logic?

My understanding of the services directory is to group any logic that is related to communicating with external services, e.g. API calls queries in that directory.

[TorstenStueber]

Arrow functions

@Sharqiewicz about arrow functions. Sure they handle this differently, but that should not matter for a React component as you will not use this nowadays within React components.

It's purely a matter of taste what you find cleaner:

const MyComponent: FC<MyComponentProps> = ({ prop1, prop2 }) => { ... }
function MyComponent({ prop1, props2 }: MyComponentProps) { ... }

Or also destructuring the argument within the component as @ebma proposes. I didn't see the style that you use before that refers to FC. Any advantage for this?

Structure

For the structure: looks good. What is utils vs services?

My understanding of the services directory is to group any logic that is related to communicating with external services

Makes sense. So should modal be moved outside of the services folder?

Exports

I also agree on named exports.

Components

What exactly do you mean with subdividing?

@ebma yes, instead of having one flat components folder where every component is a file (or folder if it comprises multiple files) in the main folder, we could group similar components in subfolders. Might make it easier to navigate. But no strong opinion.

[prayagd]

Should we estimate the work that is done here?

[ebma]

@Sharqiewicz should we add a link to the Notion page you created for the guidelines?