-
Notifications
You must be signed in to change notification settings - Fork 9
Setup Storybook for HTML mails in Demo-API #4954
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?
Conversation
203df47 to
aaa7dcc
Compare
aaa7dcc to
c551ab1
Compare
| "comet.core.deleteMutation.promptDelete": "Delete data?", | ||
| "comet.core.deleteMutation.yes": "Yes", | ||
| "comet.core.deleteMutation.no": "No", | ||
| "comet.core.dirtyHandler.discardChanges": "Discard unsaved changes?", | ||
| "comet.core.editDialog.edit": "Edit", | ||
| "comet.core.editDialog.add": "Add", | ||
| "comet.core.editDialog.cancel": "Cancel", | ||
| "comet.core.editDialog.save": "Save", | ||
| "comet.core.finalForm.abort": "Cancel", | ||
| "comet.core.finalForm.save": "Save", | ||
| "comet.core.router.confirmationDialog.confirm": "OK", | ||
| "comet.core.router.confirmationDialog.abort": "Cancel", | ||
| "comet.core.stack.stack.back": "Back", | ||
| "comet.core.table.addButton": "Add", | ||
| "comet.core.table.excelExportButton": "Export", | ||
| "comet.core.table.deleteButton": "Delete", | ||
| "comet.core.table.pagination.pageInfo": "Page {current} of {total}", | ||
| "comet.core.table.localChangesToolbar.save": "Save", | ||
| "comet.core.table.localChangesToolbar.unsavedItems": | ||
| "{count, plural, =0 {no unsaved changes} one {# unsaved change} other {# unsaved changes}}", | ||
| "comet.core.table.tableFilterFinalForm.resetButton": "Reset Filter", | ||
| "comet.core.table.tableQuery.error": "Error :( {error}", |
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.
These are the Admin messages. Could we just provide empty objects for now?
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 just copied this from the existing storybook configurations 😅
But you're right, we can probably just use an empty object.
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.
Will be deploy this to Netlify or Chromatic? If Chromatic, then we can remove this badge.
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 don't really see the need to deploy this for now, I'll remove it.
(I just copied this from the other storybook that is deployed to Chromatic, we probably should remove it there as well)
| @@ -0,0 +1,3 @@ | |||
| { | |||
| "presets": ["@comet/admin-babel-preset"] | |||
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 seems wrong, why is this necesssary?
| "ts-node": "^10.9.2", | ||
| "tsconfig-paths": "^3.15.0", | ||
| "eslint-plugin-storybook": "^9.1.10", | ||
| "storybook": "^9.1.10", |
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.
Storybook V10 is already available. Should we update to the latest version?
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 wanted to keep it the same as in the other packages to:
- avoid avoid issues due to installing multiple versions of the same package (I remember this being an issue in the past, not sure if this is still relevant)
- I assumed this was necessary to include the stroybook in the global storybook
| @@ -1,5 +1,6 @@ | |||
| import type { StorybookConfig } from "@storybook/react-webpack5"; | |||
| import remarkGfm from "remark-gfm"; | |||
| import { type StorybookConfigRaw } from "storybook/internal/types"; | |||
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.
Using an internal type is risky. Could we instead use something from Storybook's public API?
| if (configType === "DEVELOPMENT") { | ||
| refs["demo/api"] = { | ||
| title: "demo/api", | ||
| url: "http://localhost:4004/", | ||
| }; | ||
| } |
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 add this Storybook to the global Storybook?
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.
Good point, maybe this is unnecessary 🤔
| { | ||
| name: "storybook-demo-api", | ||
| script: "pnpm --filter comet-demo-api run storybook", | ||
| group: ["storybook", "docs"], |
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.
Shouldn't this be in the demo-api group instead? IMO it doesn't make sense to add this to the global Storybook as it's a Demo API specific feature.
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.
Good point, will change this
|
I'd prefer if the Demo API Storybook was something only the Demo API used, not the global Storybook. |
Description
This storybook will be used to develop HTML mails and mail-components, in future PRs.
It will also serve as an example of how a storybook for HTML mails could be implemented in a project.
Acceptance criteria
Further information