-
Notifications
You must be signed in to change notification settings - Fork 30
Better CMS Starter #222
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
base: main
Are you sure you want to change the base?
Better CMS Starter #222
Conversation
starters/cms/src/data.ts
Outdated
existingFields as ManagedCollectionFieldInput[], | ||
slugField as ManagedCollectionFieldInput |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do you need to cast the types here? Casts are dangerous because you are overriding something which might not be true
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've removed the typecasting for slugField
. But I struggle with existingFields
, I might need some help since those types are not documented.
collection.getField()
returns ManagedCollectionField[]
meawnhile collection.setFields
expects ManagedCollectionFieldInput[]
starters/cms/src/data.ts
Outdated
function slugify(text: string) { | ||
text = text.trim() | ||
text = text.slice(0, 60) // limit to 60 characters | ||
|
||
if (slugs.has(text)) { | ||
const count = slugs.get(text) ?? 0 | ||
slugs.set(text, count + 1) | ||
text = `${text} ${count + 1}` | ||
} else { | ||
slugs.set(text, 0) | ||
} | ||
|
||
const slug = text | ||
.replace(/^\s+|\s+$/g, "") | ||
.toLowerCase() | ||
.replace(/[^a-z0-9 -]/g, "") | ||
.replace(/\s+/g, "-") | ||
.replace(/-+/g, "-") | ||
.replace(/-+/g, "-") | ||
return slug | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A slugify
function should not have side effects like making something unique. If someone starts reusing this function for something different it could easily lead to bugs
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've fixed it by copying the text
variable to another one to make it safer.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Strings are value types in JS. So they can't be mutated. That's not the issue. The issue is that a slugify
function should not add suffixes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is our implementation btw:
// Note: We don't use the "slugify" package here because we want a very specific
// behavior for our CMS and web pages. Make sure to pick the right slugify
// function for your use case!
// Match everything except for letters, numbers and parentheses.
const nonSlugCharactersRegExp = /[^\p{Letter}\p{Number}()]+/gu
// Match leading/trailing dashes, for trimming purposes.
const trimSlugRegExp = /^-+|-+$/gu
/**
* Takes a freeform string and removes all characters except letters, numbers,
* and parentheses. Also makes it lower case, and separates words by dashes.
* This makes the value URL safe.
*/
export function slugify(value: string): string {
return value.toLowerCase().replace(nonSlugCharactersRegExp, "-").replace(trimSlugRegExp, "")
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need to make slugs unique at some point in case users select a field that is not unique. What the solution here, should we rename the function uniqueSlugify
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Even if you name it uniqueSlugify
, it will be very easy to use it wrong
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And that mostly means the function is too magic — as in having unexpected side effects.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Valid point, slugs
map should be reset after each batched usage then?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indeed. I would create two functions slufigy
and createUniqueSlug
. slugify
only does one job. And createUniqueSlug
takes both the raw string value and a Set with taken slugs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you @jonastreub this commit should be better:
946a468#diff-e1867fda29cfd75d20d909c3895b73c2878e32c5c28bc70912a893251891c2a1
starters/cms/src/utils.ts
Outdated
export function createUniqueSlug(text: string, existingSlugs: Map<string, number>) { | ||
text = text.trim().slice(0, 60) | ||
|
||
if (existingSlugs.has(text)) { | ||
const count = existingSlugs.get(text) ?? 0 | ||
existingSlugs.set(text, count + 1) | ||
text = `${text} ${count + 1}` | ||
} else { | ||
existingSlugs.set(text, 0) | ||
} | ||
|
||
return slugify(text) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This can still result in duplicate slugs, because we check the input instead of the generated slug
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
True, i've made it more robust now. First, input get slugified then function makes it unique.
a3e3b8c#diff-e1867fda29cfd75d20d909c3895b73c2878e32c5c28bc70912a893251891c2a1
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The function still has bugs. And I'm wondering if automatically making it unique is better than telling the user about the duplicate slug so they can fix it themselves. Having a duplicate slug is probably a mistake and if we don't warn them you will publish a URL that you will probably need to change once you see the mistake.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The bug is that we don't check the final generated slug for uniqueness. So if my data source has the following slugs it will result in duplicates:
my-slug
my-slug-0
my-slug
→ transformed intomy-slug-0
which is a duplicate
Why would you want to show the id? This is typically hidden from users |
starters/cms/src/data.ts
Outdated
await syncCollection(collection, dataSource, existingFields, slugField) | ||
await syncCollection(collection, dataSource, existingFields as ManagedCollectionFieldInput[], slugField) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This as ManagedCollectionFieldInput[]
cast is a bad habit, so should definitely not be in the starter
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is required since collection.getField()
returns ManagedCollectionField[]
meawnhile collection.setFields
expects ManagedCollectionFieldInput[]
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Casts are never the solution though. It silences an actual issue.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Of course you can use casts in your private code but this is an example where the code quality should be really high
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Btw, the fact that it currently errors is really bad… but adding a cast only hides the actual issue that we should fix
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I fixed it, no more typescating:
const fields: ManagedCollectionFieldInput[] = []
for (const field of existingFields) {
if (field.type === "multiCollectionReference" || field.type === "collectionReference") {
fields.push({
id: field.id,
name: field.name,
type: field.type,
collectionId: field.collectionId,
})
} else if (field.type === "enum") {
fields.push({
id: field.id,
name: field.name,
type: field.type,
cases: [],
})
} else if (field.type === "file" || field.type === "image") {
fields.push({
id: field.id,
name: field.name,
type: field.type,
allowedFileTypes: [],
})
} else {
fields.push({
id: field.id,
name: field.name,
type: field.type,
})
}
}
I needed to define the id field to make it different from the slug since it's necessary to keep it consistent for mapping |
starters/cms/src/data.ts
Outdated
cases: [], | ||
}) | ||
} else if (field.type === "file" || field.type === "image") { | ||
fields.push({ | ||
id: field.id, | ||
name: field.name, | ||
type: field.type, | ||
allowedFileTypes: [], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The empty arrays are incorrect
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yo @jonastreub, sorry it's a bit hard to solve this since i can't find documentation, but let me know if this commit works
Can you be more specific? The ids are supposed to be coming from your external data source. So in theory you would not need to read anything from Framer data |
Here is an example using Greenhouse Job Board API: Jobs:
Departments:
So in Framer CMS it will be two collections: Jobs & Departements where a Job reference to Departements through BTW the end user of the plugin will never have to choose what's the id field. It's a choice that the CMS starter user will do to ensure data coherence depending on the data that needs to be mapped. You can check my Greenhouse plugin that has been built on top of CMS starter. And more specifically data source folder. |
This pull request introduces several enhancements to the CMS starter, focusing on improved data modeling, field mapping, and synchronization functionality. Key changes include adding support for
collectionReference
andmultiCollectionReference
fields, introducing a more robust ID and slug handling mechanism, and making theFieldMapping
component more flexible and user-friendly.Field mapping enhancements:
FieldMapping.tsx
to include astyle
prop for customization and improved handling of missing references inFieldMappingRow
. Missing references are now displayed as "Missing Collection" and visually disabled.stroke-linecap
,stroke-linejoin
, andstroke-width
with their camelCase equivalents (strokeLinecap
,strokeLinejoin
,strokeWidth
) for better React compatibility.Data synchronization improvements:
idField
andslugField
to theDataSource
type to standardize ID and slug handling. Introduced aslugify
function to ensure slugs are unique and URL-safe.syncCollection
function to handlecollectionReference
andmultiCollectionReference
fields by mapping them to the correct collection IDs, skipping items without valid IDs, and generating slugs dynamically.Codebase refactoring:
ExtendedEditableManagedCollectionField
type to extend field capabilities, including support fordataSourceId
andisMissingReference
.