feat: Add the Marketing Banner component #218
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Deploying ui2 with
|
| Latest commit: |
b535645
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://dbdacd14.ui2-423.pages.dev |
| Branch Preview URL: | https://feat-add-marketing-banner.ui2-423.pages.dev |
braianj
approved these changes
Jan 10, 2025
| margin: 0, | ||
| padding: 0, | ||
| }, | ||
| "@media (max-width: 767px)": { |
There was a problem hiding this comment.
you can use the breakpoints from the theme here as this, what do you think?
Suggested change
| "@media (max-width: 767px)": { | |
| [theme.breakpoints.down("sm")]: { |
| display: "flex", | ||
| marginTop: "1rem", | ||
| alignItems: convertAlignmentToFlex(desktopAlignment || "left"), | ||
| "@media (max-width: 767px)": { |
There was a problem hiding this comment.
Same as before
Suggested change
| "@media (max-width: 767px)": { | |
| [theme!.breakpoints.down("sm")]: { |
Comment on lines
92
to
110
| const Text = styled(Box, { | ||
| shouldForwardProp: (prop) => | ||
| prop !== "mobileTextAlignment" && prop !== "desktopTextAlignment", | ||
| })<{ | ||
| mobileTextAlignment?: Property.TextAlign | ||
| desktopTextAlignment?: Property.TextAlign | ||
| }>(({ mobileTextAlignment, desktopTextAlignment }) => ({ | ||
| color: "#fff", | ||
| textAlign: desktopTextAlignment || "left", | ||
| fontSize: "19px", | ||
| "& p": { | ||
| margin: 0, | ||
| padding: 0, | ||
| }, | ||
| "@media (max-width: 767px)": { | ||
| textAlign: mobileTextAlignment || "left", | ||
| fontSize: "16px", | ||
| }, | ||
| })) |
There was a problem hiding this comment.
WDYT about extracting the props inside the function to follow the new standard
Suggested change
| const Text = styled(Box, { | |
| shouldForwardProp: (prop) => | |
| prop !== "mobileTextAlignment" && prop !== "desktopTextAlignment", | |
| })<{ | |
| mobileTextAlignment?: Property.TextAlign | |
| desktopTextAlignment?: Property.TextAlign | |
| }>(({ mobileTextAlignment, desktopTextAlignment }) => ({ | |
| color: "#fff", | |
| textAlign: desktopTextAlignment || "left", | |
| fontSize: "19px", | |
| "& p": { | |
| margin: 0, | |
| padding: 0, | |
| }, | |
| "@media (max-width: 767px)": { | |
| textAlign: mobileTextAlignment || "left", | |
| fontSize: "16px", | |
| }, | |
| })) | |
| const Text = styled(Box, { | |
| shouldForwardProp: (prop) => | |
| prop !== "mobileTextAlignment" && prop !== "desktopTextAlignment", | |
| })<{ | |
| mobileTextAlignment?: Property.TextAlign | |
| desktopTextAlignment?: Property.TextAlign | |
| }>((props) => { | |
| const { mobileTextAlignment, desktopTextAlignment, theme } = props | |
| return { | |
| color: "#fff", | |
| textAlign: desktopTextAlignment || "left", | |
| fontSize: "19px", | |
| "& p": { | |
| margin: 0, | |
| padding: 0, | |
| }, | |
| [theme.breakpoints.down("sm")]: { | |
| textAlign: mobileTextAlignment || "left", | |
| fontSize: "16px", | |
| }, | |
| } | |
| }) |
| })<{ | ||
| mobileAlignment?: Property.TextAlign | ||
| desktopAlignment?: Property.TextAlign | ||
| }>(({ mobileAlignment, desktopAlignment }) => ({ |
There was a problem hiding this comment.
same here about the props
Comment on lines
25
to
31
| export const MarketingBanner: React.FC<MarketingBannerProps> = ({ | ||
| id, | ||
| environment, | ||
| token, | ||
| space, | ||
| locale = Locales.enUS, | ||
| }) => { |
There was a problem hiding this comment.
Same here about extracting the props inside the function so we have consistency throw all the repo
src/hooks/contenful/contentful.ts
Outdated
| `https://cdn.contentful.com/spaces/${space}/environments/${environment}/entries/?` + | ||
| new URLSearchParams({ | ||
| "sys.id": id, | ||
| content_type: "banner", |
There was a problem hiding this comment.
can I use useGetContentfulEntry to get other kind of entries, right?
Maybe we can pass it throw the props or, change the same of the hook to useGetContentfulBannerEntry. Or even remove the content_type
There was a problem hiding this comment.
Changed it so any content type can be used.
src/hooks/contenful/contentful.ts
Outdated
| ) | ||
|
|
||
| if (!response.ok) { | ||
| throw new Error("Failed to fetch banner data") |
There was a problem hiding this comment.
maybe If we make any of the changes above, we'll need to change these message
braianj
approved these changes
Jan 10, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR creates a new component, the MarketingBanner, used to show via Contentful a banner based on its entity id.
It also adds a hook to retrieve information from a Contentful entity that can be used for other purposes.