-
-
Notifications
You must be signed in to change notification settings - Fork 28
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
Set cookie with location of request country #992
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
Visit the preview URL for this PR (updated for commit b69b72d): https://si-admin-staging--pr992-michael-country-cook-6rk4yyy3.web.app (expires Sun, 29 Dec 2024 15:10:28 GMT) 🔥 via Firebase Hosting GitHub Action 🌎 Sign: b7b0969384059dce6ea8fad1ee1d1737e54e6676 |
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.
Hey @mkue, this is a great idea 💡 I didn’t think of the middleware approach!
However, the flag should follow the region when it’s not /int
(which we have /ch
for now). That’s why I checked it using isIntRegion = region === 'int'
, can we bring that back or you can suggest a better way.
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.
Thanks. Yes we should focus on location (and ignore currency for the flag). /ch should always show Swiss flag. /int should always show actual location
Yes, that makes total sense. Sorry, I should not have removed that. I put it back here. |
I'll approve the other MR. Feel free to merge it once this is merged into your branch. All looks good to me. Thanks a lot for your help! |
* Install Vercel functions * Make flag image url global * Display user country flag * Integrate geolocation API * Update image * Disable query stale * Remove currency flag fallback * Limit field * Show flag for region * Relocate hook * Sort lines * Set cookie with location of request country (#992) * Set cookie with location of request country * fix typing issue * Prioritize region if not set to 'int' * Fix image positioning --------- Co-authored-by: Michael Kündig <michael@socialincome.org>
Hey @dnhn! Looking at your #986 MR again, I realized that we can already set the country code of the request location as a cookie in the middleware. Doing it like this, it's not necessary anymore to do an extra request to the API endpoint you created because you can directly access the info in the cookie.
What do you think about this approach?
Seems to work as well: