-
Notifications
You must be signed in to change notification settings - Fork 21
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 a CMS Starter example #134
base: main
Are you sure you want to change the base?
Conversation
ce26e37
to
fc608ad
Compare
fc608ad
to
36e36fc
Compare
36e36fc
to
354b9f5
Compare
Signed-off-by: Cédric Boirard <[email protected]>
Signed-off-by: Cédric Boirard <[email protected]>
64eda3c
to
405e0df
Compare
4104a2f
to
ba7fd80
Compare
ba7fd80
to
d5d0288
Compare
Signed-off-by: Cédric Boirard <[email protected]>
d5d0288
to
80d249f
Compare
- Chore Signed-off-by: Cédric Boirard <[email protected]>
Lets call the actual folder "cms" instead of "cms-starter" since its already in a folder called "examples" |
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.
Sorry if the amount of feedback seems overwhelming. Since this will be the starter kit for many customers I like it to become as simple and clear as we can possibly make it. And it's getting really good already 👏
Signed-off-by: Cédric Boirard <[email protected]>
ec9967f
to
1bea768
Compare
if (!isLoadingConfiguredDataSource && !dataSource) { | ||
return <SelectDataSource dataSourceIds={getDataSourcesIds()} onSelectDataSource={setDataSource} /> | ||
} | ||
|
||
if (isLoadingConfiguredDataSource) { | ||
return <Spinner /> | ||
} |
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.
By moving the rendering of Spinner
up before SelectDataSource
, you don't need to check for !isLoadingConfiguredDataSource
if (!previouslyConfiguredDataSourceId || hasStartedLoadingDataSource.current) { | ||
return | ||
} |
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 the logic for hasStartedLoadingDataSource
? The value of previouslyConfiguredDataSourceId
can't change at runtime, right?
useLayoutEffect(() => { | ||
framer.showUI({ | ||
width: hasDataSourceSelected ? 360 : 320, | ||
height: hasDataSourceSelected ? 425 : 305, | ||
minWidth: hasDataSourceSelected ? 360 : undefined, | ||
minHeight: hasDataSourceSelected ? 425 : undefined, | ||
resizable: dataSource !== null, |
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.
Any reason we use hasDataSourceSelected
for all layout states except for resizable
?
if (dataSource) { | ||
return ( | ||
<FieldMapping | ||
collection={collection} | ||
dataSource={dataSource} | ||
initialSlugFieldId={previouslyConfiguredSlugFieldId} | ||
/> | ||
) | ||
} | ||
|
||
assertNever( | ||
`Invalid state: ${JSON.stringify({ | ||
previouslyConfiguredDataSourceId, | ||
isLoadingConfiguredDataSource, | ||
dataSource, | ||
})}` | ||
) |
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 think the assertNever
looks way too scary for something that should never happen anyway. You already have an early return when there is no data source
const assertNever = (message: string): never => { | ||
throw new Error(message) |
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.
Let's remove this
if (!selectedSlugField) { | ||
// This can't happen because the form will not submit if no slug field is selected | ||
// but TypeScript can't infer that. | ||
console.error("There is no slug field selected. Sync will not be performed") |
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.
Should we show a toast? I think user feedback would be useful
<select | ||
name="slugField" | ||
className="field-input" | ||
value={selectedSlugField ? selectedSlugField.id : ""} | ||
onChange={event => | ||
setSelectedSlugField( | ||
possibleSlugFields.find(field => field.id === event.target.value) ?? null | ||
) | ||
} | ||
required |
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.
Nit. I would move attributes without a value like required
all the way up, that way it's easier to read without them hanging in the void.
onChange={event => | ||
setSelectedSlugField( | ||
possibleSlugFields.find(field => field.id === event.target.value) ?? null | ||
) | ||
} |
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 be simplified by doing only one operation per line:
const selectedFieldId = event.target.value
const selectedField = possibleSlugFields.find(field => field.id === selectedFieldId)
if (!selectedField) return
setSelectedSlugField(selectedField)
const handleFieldNameChange = (fieldId: string, name: string) => { | ||
setFields(previousFields => previousFields.map(field => (field.id === fieldId ? { ...field, name } : field))) | ||
} |
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 works but it's hard to read. Let's do the same thing again, one operation per line.
const updatedFields = previousFields.map(field => {
if (field.id !== fieldId) return field
return {...field, name }
})
setFields(updatedFields)
onChange={event => { | ||
const value = event.target.value | ||
if (!value.trim()) { | ||
setHasCustomName(false) | ||
onFieldNameChange(field.id, field.id) | ||
} else { | ||
setHasCustomName(true) | ||
onFieldNameChange(field.id, value.trimStart()) | ||
} | ||
}} |
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.
Can we get rid of all special casing here? And by default have empty strings and you can type whatever you want and also manually clear the values, and then on sync we check if the value is valid or different?
interface FieldMappingProps { | ||
collection: ManagedCollection | ||
dataSource: DataSource | ||
initialSlugFieldId: string | null | ||
} | ||
|
||
type FieldMappingStatus = "mapping-fields" | "loading-fields" | "syncing-collection" | ||
|
||
export function FieldMapping({ collection, dataSource, initialSlugFieldId }: FieldMappingProps) { |
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.
Nit. Since the Props are so important to the component I would move that next to the component and have other types like FieldMappingStatus
be further away. I think it would also be fine to have that type inline since it isn't used anywhere else
useEffect(() => { | ||
collection | ||
.getFields() | ||
.then(fields => { | ||
setFields(mergeFieldsWithExistingFields(sourceFields, fields)) | ||
|
||
if (Boolean(initialSlugFieldId) === false && fields.length === 0) { | ||
return | ||
} | ||
|
||
const ignoredFields = sourceFields.filter( | ||
field => !fields.some(existingField => existingField.id === field.id) | ||
) | ||
|
||
setIgnoredFieldIds(new Set(ignoredFields.map(field => field.id))) | ||
}) | ||
.catch(error => { | ||
console.error(error) | ||
framer.notify("Failed to load existing fields.", { variant: "error" }) | ||
}) | ||
.finally(() => { | ||
setStatus("mapping-fields") | ||
}) | ||
}, [collection, sourceFields, initialSlugFieldId]) |
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 effect can run multiple times and it is doing something async, so we need to be able to cancel it when the effect is cleaned up
const [fields, setFields] = useState<ManagedCollectionField[]>([]) | ||
const [ignoredFieldIds, setIgnoredFieldIds] = useState<Set<string>>(new Set()) |
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 thin it would be good to mark these as readonly. And I wonder if the defaults should be lifted outside of rendering, so they don't have to be recreated on every render.
// Outside of rendering
type ManagedCollectionFields = readonly ManagedCollectionField[]
const initialManagedCollectionFields: ManagedCollectionFields = []
type FieldId = string
type FieldIds = ReadonlySet<FieldId>
const initialFieldIds: FieldIds = new Set()
// Within the component
const [fields, setFields] = useState<ManagedCollectionFields>(initialManagedCollectionFields)
const [ignoredFieldIds, setIgnoredFieldIds] = useState<FieldIds>(initialFieldIds)
useEffect(() => { | ||
collection | ||
.getFields() | ||
.then(fields => { | ||
setFields(mergeFieldsWithExistingFields(sourceFields, fields)) | ||
|
||
if (Boolean(initialSlugFieldId) === false && fields.length === 0) { |
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 would move the check if there is no initialSlugFieldId
to the beginning of the effect, because that means we don't need to do any work
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 I don't think we need to check for a length of zero. Fine to set an empty array again. No reason to add extra conditionals for
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.
Oh wait. I missed that it's &&
. So you can't do that
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 still don't understand this early return though
const ignoredFields = sourceFields.filter( | ||
field => !fields.some(existingField => existingField.id === field.id) | ||
) |
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.
Let's name what's happening here by doing one operation per line with clearly named variables
field => !fields.some(existingField => existingField.id === field.id) | ||
) | ||
|
||
setIgnoredFieldIds(new Set(ignoredFields.map(field => field.id))) |
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.
If you create a utility named getId
you can use it in a tone of places like this: ignoredFields.map(getId)
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 have a similar utility in Vekter
await syncCollection( | ||
collection, | ||
dataSource, | ||
fields.filter(field => !ignoredFieldIds.has(field.id)), |
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.
Let's move this out into a variable, something like:
const fieldsToUpdate = fields.filter(field => !ignoredFieldIds.has(field.id))
function Logo() { | ||
return ( | ||
<div className="logo"> | ||
<svg xmlns="http://www.w3.org/2000/svg" viewBox="0 0 150 150"> | ||
<path | ||
fill="#999" | ||
d="M75 33c18.778 0 34 7.611 34 17S93.778 67 75 67s-34-7.611-34-17 15.222-17 34-17Zm34 40.333C109 82.538 93.778 90 75 90s-34-7.462-34-16.667V60c0 9.389 15.222 17 34 17 18.776 0 33.997-7.61 34-16.997v13.33Z" | ||
/> | ||
</svg> | ||
</div> | ||
) | ||
} |
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 would make icon components just the SVG, no other HTML tags. Logo is a generic name, maybe the name of the component could be more descriptive so users will recognize it.
const activeCollection = await framer.getManagedCollection() | ||
|
||
const previouslyConfiguredDataSourceId = await activeCollection.getPluginData(PLUGIN_KEYS.DATA_SOURCE_ID) | ||
const previouslyConfiguredSlugFieldId = await activeCollection.getPluginData(PLUGIN_KEYS.SLUG_FIELD_ID) | ||
|
||
const { didSync } = await syncExistingCollection( | ||
activeCollection, | ||
previouslyConfiguredDataSourceId, | ||
previouslyConfiguredSlugFieldId | ||
) | ||
|
||
if (didSync) { | ||
await framer.closePlugin(`Synchronization successful`, { | ||
variant: "success", | ||
}) | ||
} else { | ||
const root = document.getElementById("root") | ||
if (!root) throw new Error("Root element not found") | ||
|
||
ReactDOM.createRoot(root).render( | ||
<React.StrictMode> | ||
<App | ||
collection={activeCollection} | ||
previouslyConfiguredDataSourceId={previouslyConfiguredDataSourceId} | ||
previouslyConfiguredSlugFieldId={previouslyConfiguredSlugFieldId} | ||
/> | ||
</React.StrictMode> | ||
) | ||
} |
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 reads like a poem ❤️
Getting really good 💪 |
Description
This pull request introduces a new CMS starter example, including updates to various configuration files, data sources, and UI components. The changes aim to set up a basic CMS plugin for Framer, providing a template for further development.
QA
examples/cms
directory.Additional Checks