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

Remove email campaigns factory #148

Open
wants to merge 5 commits into
base: next
Choose a base branch
from

Conversation

raphaelblum
Copy link
Collaborator

@raphaelblum raphaelblum commented Nov 15, 2024

depends on !147

COM-1053

Description

The factory is not needed. I'd like to simplify creating the EmailCampaignsPage.

@raphaelblum raphaelblum force-pushed the remove-email-campaigns-factory branch from 29758fb to dd52872 Compare November 15, 2024 12:27
primary: <FormattedMessage id="menu.newsletter.emailCampaigns" defaultMessage="Email campaigns" />,
route: {
path: "/newsletter/email-campaigns",
component: () => <EmailCampaignsPage EmailCampaignContentBlock={EmailCampaignContentBlock} />,
Copy link
Member

@thomasdax98 thomasdax98 Nov 20, 2024

Choose a reason for hiding this comment

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

Suggested change
component: () => <EmailCampaignsPage EmailCampaignContentBlock={EmailCampaignContentBlock} />,
render: () => <EmailCampaignsPage EmailCampaignContentBlock={EmailCampaignContentBlock} />,

This must be render or the component will unmount on every render: https://v5.reactrouter.com/web/api/Route/component#:~:text=That%20means%20if%20you%20provide%20an%20inline%20function%20to%20the%20component%20prop%2C%20you%20would%20create%20a%20new%20component%20every%20render.

"@comet/brevo-admin": major
---

Remove factory createEmailCampaignsPage
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
Remove factory createEmailCampaignsPage
Remove `createEmailCampaignsPage` factory

@RainbowBunchie
Copy link
Contributor

This needs a rebase @raphaelblum 🙏🏻

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.

3 participants