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

GEN-637 - feat: make Trustpilot data available globally #2710

Merged
merged 1 commit into from
Jul 10, 2023

Conversation

guilhermespopolin
Copy link
Contributor

@guilhermespopolin guilhermespopolin commented Jul 7, 2023

Describe your changes

  • Fetch Trustpilot data for Storyblok pages and make it available globally.

How the whole thing works:

  1. There's a new service - apps/store/src/services/trustpilot/trustpilot.tsx - which fetch that data from Trustpilot API and store it in a Jotai's atom so it can be available through the whole app. We then fetch that data and proper hydrate the atom in the apps/store/src/pages/[[...slug]].tsx and apps/store/src/components/LayoutWithMenu/LayoutWithMenu.tsx files respectively.
  2. TrustpilotBlock then reads that data from the atom and proper display it.
Screenshot 2023-07-06 at 17 41 54

Justify why they are needed

Requested by design/editors

Can be tested here

@vercel
Copy link

vercel bot commented Jul 7, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
hedvig-dot-com ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jul 10, 2023 0:48am
onboarding ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jul 10, 2023 0:48am

@guilhermespopolin
Copy link
Contributor Author

@guilhermespopolin guilhermespopolin changed the title feat: make Trustpilot data available globally GEN-637 - feat: make Trustpilot data available globally Jul 7, 2023
@notion-workspace
Copy link

Copy link
Contributor

@robinandeer robinandeer left a comment

Choose a reason for hiding this comment

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

Hmm now I'm confused 🤔

  1. We fetch and store Trustpilot data in edge config
  2. We fetch it again during build and cache it using SSG

Seems like we are just caching in two different ways, no? Could we not just fetch from Trustpilot at build-time and skip edge config?

@guilhermespopolin guilhermespopolin force-pushed the GEN-637/feat/trustpilot-block_split_1 branch from 4f41042 to 727aa26 Compare July 7, 2023 15:51
@guilhermespopolin
Copy link
Contributor Author

Hmm now I'm confused 🤔

  1. We fetch and store Trustpilot data in edge config
  2. We fetch it again during build and cache it using SSG

Seems like we are just caching in two different ways, no? Could we not just fetch from Trustpilot at build-time and skip edge config?

You right 👍🏻
Have updated the code by fetching it directly from their API for Storyblok pages. We've talked about rate limiting but it seems fetch requests are cached by default by next. We could only fetch it for pages that include the block but that would complicate getStaticProps a little bit so I decided to give it a go as it is and see what happens. It's the simplest solution :)

Copy link
Contributor

@robinandeer robinandeer left a comment

Choose a reason for hiding this comment

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

Looks good, just had a comment about where to rehydrate

@@ -33,6 +34,8 @@ export const LayoutWithMenu = (props: LayoutWithMenuProps) => {
const { story, globalStory, className, breadcrumbs } = props.children.props

useHydrateProductMetadata(props.children.props[GLOBAL_PRODUCT_METADATA_PROP_NAME])
useHydrateTrustpilotData(props.children.props.trustpilot)
Copy link
Contributor

Choose a reason for hiding this comment

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

hmm can we place this in a more relevant place? This has nothing to do with the "layout with menu" no?

Perhaps we can hydrate on "[[...slug]]" page instead? where we fetch the data anyways?

@guilhermespopolin
Copy link
Contributor Author

@guilhermespopolin started a stack merge that includes this pull request via Graphite.

Base automatically changed from GEN-637/feat/trustpilot-block to main July 10, 2023 12:41
@guilhermespopolin
Copy link
Contributor Author

Graphite rebased this pull request as part of a merge.

@guilhermespopolin guilhermespopolin force-pushed the GEN-637/feat/trustpilot-block_split_1 branch from 91abec3 to 0e6b172 Compare July 10, 2023 12:41
@guilhermespopolin guilhermespopolin merged commit 327a5bc into main Jul 10, 2023
3 checks passed
@guilhermespopolin
Copy link
Contributor Author

@guilhermespopolin merged this pull request with Graphite.

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.

2 participants