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

Add support for parsing layouts #370

Merged
merged 14 commits into from
Sep 14, 2023
Merged

Add support for parsing layouts #370

merged 14 commits into from
Sep 14, 2023

Conversation

nmanu1
Copy link
Contributor

@nmanu1 nmanu1 commented Sep 12, 2023

This PR creates a new LayoutMetadata domain model to represent a Layout. A new LayoutFile sourcefile is added for parsing layouts. Currently, we only support layouts in the top-level of the src/layouts directory (i.e. no nested folders). Any nested directories for both layout and pages will now be ignored.

I opted not to create a model for layouts with errors because, for now, this information is not anticipated to be used in the UI. Like components, we will not be showing layouts with errors in the Add Element menu. Unlike components, there is nowhere else in the UI an errored layout would surface, since it's not a "component" that would be part of a component tree. The parsing error is still logged in the terminal though.

J=SLAP-2929
TEST=auto

Since much of the logic is shared between parsing a page and parsing a layout, I moved the tests for parsing a component tree into a separate file so only sourcefile-specific tests remain in PageFile.getPageState.test.tsx.

packages/studio-plugin/src/ParsingOrchestrator.ts Outdated Show resolved Hide resolved
packages/studio-plugin/src/ParsingOrchestrator.ts Outdated Show resolved Hide resolved
packages/studio-plugin/src/ParsingOrchestrator.ts Outdated Show resolved Hide resolved
packages/studio-plugin/src/ParsingOrchestrator.ts Outdated Show resolved Hide resolved
packages/studio-plugin/src/sourcefiles/LayoutFile.ts Outdated Show resolved Hide resolved
packages/studio-plugin/src/sourcefiles/LayoutFile.ts Outdated Show resolved Hide resolved
componentTree: ComponentState[];
cssImports: string[];
filepath: string;
export type PageState = LayoutMetadata & {
Copy link
Contributor

Choose a reason for hiding this comment

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

Admittedly, this is a bit pedantic, but I think we need to be careful about how we set up the typing relationship between Static Pages, Entity Pages, and Layouts. If we're not careful, people can start conflating things (e.g. a Layout is a Static Page).

Having PageState = LayoutMetadata & { ... } sets up a relationship where a PageState "is a" (or at least "contains a") Layout. That makes me a bit nervous. It just so happens that a Layout and a Page share very similar structure, but the two serve different purposes. We want to re-use typings and do composition of types. But, the way that we compose those types does establish these sort of "is a" relationships.

Here's one half-baked idea:

export type ComponentLayout = {
  componentTree: ConmponentState[]
}

export type FileMetadata = {
  cssImports: string[],
  filepath: string;
}

export type LayoutMetadata = {
  layout: ComponentLayout,
  metadata: FileMetadata
}

export type PageState = {
  layout: ComponentLayout,
  metadata: FileMetadata,
  pagesJS?: PagesJSState
}

That is a very rough idea, we can't implement just that since we already have a FileMetadata. I think one of the key ideas is the differentiation between a Component Layout (a pre-selected set of Components) and a Layout File. A PageState will always have a Component Layout. If a PageState is initially sourced from a Layout File, there can be drift as time goes on. The PageState may keep the same Component layout, but a developer could decide to style those Components differently, changing the cssImports so they no longer match the original LayoutMetadata.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was under the impression that we wanted pages to exactly mirror layouts and that we'd always want to keep them in sync so that a layout and a non-PagesJS page would always be interchangeable. But the distinction you're making makes sense. For now, to be on the safe side I've removed the coupling between PageState and LayoutMetadata so the types are independent. It'll take a little more thought to see what the best way is to relate the types safely and coherently like in your example

Copy link
Contributor

Choose a reason for hiding this comment

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

Okay, let's keep them independent for now. We can punt on ironing out the type hierarchy. I will make an item for that.

};
}

private constructRecordsFromSourceFile<SourceFile, SuccessData>(
Copy link
Contributor

Choose a reason for hiding this comment

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

@oshi97, this method feels a bit confusing to me, especially in it's return type. I wonder if there's a better way to consolidate this logic between Layouts & Pages.

Copy link
Contributor

Choose a reason for hiding this comment

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

renamed to separateErrorAndSuccessResults! Not sure about these names but I think they're ok for now

);
}

private initNameToSourceFile<SourceFile>(
Copy link
Contributor

Choose a reason for hiding this comment

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

@oshi97, for this method, I think all we need is a more intuitive name.

Copy link
Contributor

Choose a reason for hiding this comment

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

renamed to createFilenameMapping!

this.layoutNameToLayoutFile = this.initLayoutNameToLayoutFile();
}

getLayoutNameToLayoutState() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Could we add a return type signature here?

@oshi97 oshi97 merged commit c30b717 into main Sep 14, 2023
15 checks passed
@oshi97 oshi97 deleted the dev/layout-parsing branch September 14, 2023 18:24
@tmeyer2115 tmeyer2115 mentioned this pull request Sep 22, 2023
tmeyer2115 added a commit that referenced this pull request Sep 22, 2023
## Features
- We've introduced the concept of Page Layouts (or Layouts) to Studio. A
Page Layout is a pre-prescribed set of Components that you can apply to
a Page. By default, Layouts live in the `src/layouts` directory. In the
UI, a Layout can be applied via the "Insert" button. (#370, #367, #375)
- There is now improved UX for creating or editing an Entity Page's
Stream Scope. Instead of being raw inputs, the fields for Entity Ids,
Entity Types, and Saved Filters are now drop-downs with the relevant
options populated from the account. (#380)

## Changes
- Repeaters and Modules have been officially deprecated and removed from
Studio. (#369)
alextaing pushed a commit that referenced this pull request Sep 22, 2023
## Features
- We've introduced the concept of Page Layouts (or Layouts) to Studio. A
Page Layout is a pre-prescribed set of Components that you can apply to
a Page. By default, Layouts live in the `src/layouts` directory. In the
UI, a Layout can be applied via the "Insert" button. (#370, #367, #375)
- There is now improved UX for creating or editing an Entity Page's
Stream Scope. Instead of being raw inputs, the fields for Entity Ids,
Entity Types, and Saved Filters are now drop-downs with the relevant
options populated from the account. (#380)

## Changes
- Repeaters and Modules have been officially deprecated and removed from
Studio. (#369)
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.

4 participants