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

Content presets #9

Merged
merged 5 commits into from
Oct 4, 2021
Merged

Content presets #9

merged 5 commits into from
Oct 4, 2021

Conversation

davbree
Copy link
Member

@davbree davbree commented Oct 3, 2021

Add support for content presets

@davbree davbree requested a review from smnh October 3, 2021 09:40
}));
expect(result.config.presets).toMatchObject(expect.objectContaining({
'.stackbit/presets/model1.json:presets[1]': expect.objectContaining({
thumbnail: '/images/preset-thumbnail2.png'
Copy link
Member

@smnh smnh Oct 3, 2021

Choose a reason for hiding this comment

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

The resolved thumbnails paths should be relative to project root, and without / prefix. See comments below for more info.
This is also because if we run path.resolve(projDir, thumbnail) it should return correct path. And path.resolve method will always ignore the projDir if thumbnail starts with /.

}));
expect(result.config.presets).toMatchObject(expect.objectContaining({
'.stackbit/presets/model1.json:presets[1]': expect.objectContaining({
thumbnail: '/images/preset-thumbnail2.png'
Copy link
Member

@smnh smnh Oct 3, 2021

Choose a reason for hiding this comment

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

this is the same test as one above it, maybe you meant model1_other?

@@ -126,6 +127,7 @@ export interface YamlBaseModel {
groups?: string[];
fieldGroups?: FieldGroupItem[];
fields?: Field[];
presets?: any;
Copy link
Member

Choose a reason for hiding this comment

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

Do we need presets on YamlBaseModel?
YamlBaseModel defines the structure of the actual yaml model file definition.
The Config interface defines the structure of the config object returned by the SDK with resolved presets.
From the code below, I see that you add presets to the variable of Config type only.

Copy link
Member Author

Choose a reason for hiding this comment

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

A model will have presets as well (preset id's), right?
Would BaseModel be better?

Copy link
Member

@smnh smnh Oct 3, 2021

Choose a reason for hiding this comment

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

oh oh, sorry I confused some stuff myself...

So, the final Config type has presets which is a map of all the presets to their IDs:

export interface Config extends BaseConfig {
models: Model[];
presets?: any;
}

👍

Then, every loaded model returned by SDK also has presets which is the array of preset IDs of a specific model.
You defined it inside YamlBaseModel:

export interface YamlBaseModel {
type: 'object' | 'data' | 'page' | 'config';
__metadata?: {
filePath?: string;
invalid?: boolean;
};
label: string;
description?: string;
thumbnail?: string;
extends?: string | string[];
labelField?: string;
variantField?: string;
groups?: string[];
fieldGroups?: FieldGroupItem[];
fields?: Field[];
presets?: any;
}

This is OK and will not cause any problems, it is just it is not strict.

In any case, FYI...

There are two main Model types:

  1. YamlModel - type of models as they should be defined in model files (and used to type Joi schemas)

    export type YamlModel = StricterUnion<YamlObjectModel | YamlDataModel | YamlPageModel | YamlConfigModel>;

  2. Model - type of models as returned by loadConfig()

    export type Model = StricterUnion<ObjectModel | DataModel | PageModel | ConfigModel>;

The "Model" basically extends the "YamlModel" plus adds the "name" field (in that "BaseModel" interface you noted). Although technically speaking, it is not true anymore :). Because separate model files do have name field, but we normalize the loaded files before validating them and convert all models into a map as if all models were defined in stackbit.yaml with model names as keys
But the "presets" are always added to the final result, so yeah, the ideal place for them will be the "BaseModel".

}

export interface PresetsLoaderResult {
config: Config | null;
Copy link
Member

@smnh smnh Oct 3, 2021

Choose a reason for hiding this comment

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

The result probably can't have null for config because it always receives a non null Config.

const presetsRelDir = path.dirname(presetFile);
const presetPath = path.join(dirPath, presetFile);
const presetData = await parseFile(presetPath);
for (const [i, preset] of presetData.presets.entries()) {
Copy link
Member

@smnh smnh Oct 3, 2021

Choose a reason for hiding this comment

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

don't assume presetData.presets will be an array or that it will even exists, use:

_.forEach(_.get(presetData, 'presets', {}), (preset, i) => { ... });

or any other defensive coding

later we can return user facing errors, but the code should not throw.


function resolveThumbnailPath(thumbnail: string, dir: string) {
if (thumbnail.startsWith('/')) {
return thumbnail;
Copy link
Member

@smnh smnh Oct 3, 2021

Choose a reason for hiding this comment

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

Lets make it to work the same way other thumbnails work.

  1. remove the / prefix if it starts with one
  2. if / is used inside component library it should resolve to component library's root

function resolveThumbnailPath(thumbnail: string, modelDirPath: string) {
if (thumbnail.startsWith('/')) {
if (modelDirPath.endsWith('@stackbit/components/models')) {
modelDirPath = modelDirPath.replace(/\/models$/, '');
} else {
modelDirPath = '';
}
thumbnail = thumbnail.replace(/^\//, '');
}
return path.join(modelDirPath, thumbnail);
}

Comment on lines 3 to 11
"presets": [
{
"label": "ext - my preset 1",
"thumbnail": "path/to/ext-preset-thumbnail1.png",
"data": {
"enum_field": "thumbnail_1"
}
}
]
Copy link
Member

@smnh smnh Oct 3, 2021

Choose a reason for hiding this comment

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

suggest adding another test that resolved absolutes path inside component library like we do with regular thumbnails:

test:

result - note, the models folder was removed due to use of /:

{
name: 'model_components_5',
thumbnail: 'node_modules/@stackbit/components/abs/path/thumbnail.png',
fields: [

The function that maps thumbnails

function resolveThumbnailPath(thumbnail: string, modelDirPath: string) {
if (thumbnail.startsWith('/')) {
if (modelDirPath.endsWith('@stackbit/components/models')) {
modelDirPath = modelDirPath.replace(/\/models$/, '');
} else {
modelDirPath = '';
}
thumbnail = thumbnail.replace(/^\//, '');
}
return path.join(modelDirPath, thumbnail);
}

Comment on lines 43 to 46
if (!presetsByModel[presetData.model]) {
presetsByModel[presetData.model] = [];
}
presetsByModel[presetData.model].push(presetId);
Copy link
Member

Choose a reason for hiding this comment

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

FYI, we have neat append util, not critical tho

import { append } from '@stackbit/utils';

append(presetsByModel, presetData.model, presetId);

Copy link
Member Author

Choose a reason for hiding this comment

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

love it

@davbree davbree requested a review from smnh October 3, 2021 11:49
@davbree davbree merged commit 1cf6c1a into master Oct 4, 2021
@davbree davbree deleted the content-presets branch October 4, 2021 10:36
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.

None yet

2 participants