fix: platform booker icons and address property for type inPerson #15207
fix: platform booker icons and address property for type inPerson #15207Ryukemeister merged 6 commits intomainfrom
Conversation
|
Thank you for following the naming conventions! 🙏 Feel free to join our discord and post your PR link. |
keithwillcode
added
core
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎ 3 Ignored Deployments
|
dosubot
Bot
added
bookings
Graphite Automations"Add platform team as reviewer" took an action on this PR • (05/27/24)1 reviewer was added to this PR based on Keith Williams's automation. |
| return { | ||
| // XYZ: is considered a namespace in i18next https://www.i18next.com/principles/namespaces and thus it get's cleaned up. | ||
| // Beacause there can be a URL in here, simply don't translate it if it starts with http: or https:. This would allow us to keep supporting namespaces if we plan to use them | ||
| label: locationString.search(/^https?:/) !== -1 ? locationString : t(locationString), |
There was a problem hiding this comment.
This conditional was the reason why the inPerson address was not being displayed. The logic above states that we only check for URL's above and keep them as it is in the locationString variable and translate all the other values of locationString variable, hence that check will only work for the type: link. So if we have a location value like { type: "address", address: "King Street 1234, London" } then our locationString value becomes King Street 1234, London and according to the above since this location doesn't contain https: it will try to translate it which will result in a blank string being returned since we don't have any value for this inside translations common.json file
There was a problem hiding this comment.
The getTranslatedLocation function that I'm using now to get the correctly translated location is already being used in the tooltip in one of the Booker components which can be found under the file packages/features/bookings/components/event-meta/AvailableEventLocations.tsx line 59
Ryukemeister
changed the title
Ryukemeister
changed the title
supalarry
left a comment
supalarry
left a comment
There was a problem hiding this comment.
Super great work Rajiv and super happy that you found what was going wrong - respect!
Tested locally and it works, besides small comment below I approve this PR.
After merge: need to update stonealgo in this thread that they can now make stuff work without a workaround.
| // XYZ: is considered a namespace in i18next https://www.i18next.com/principles/namespaces and thus it get's cleaned up. | ||
| // Beacause there can be a URL in here, simply don't translate it if it starts with http: or https:. This would allow us to keep supporting namespaces if we plan to use them | ||
| label: locationString.search(/^https?:/) !== -1 ? locationString : t(locationString), | ||
| label: translatedLocation || "", |
There was a problem hiding this comment.
Should we have:
label: translatedLocation || locationString
Because if translation returns null, then it's better we display original one to at least know what is going on, because now simply rendering empty "" makes it easier to understand where did the original "locationString".
There was a problem hiding this comment.
True it's better to return the original locationString value to understand whats going on, will fix this.
…lcom#15207) * fix: platform booker icons * fix label not being shown for address * fix type check * PR feedback --------- Co-authored-by: Morgan Vernay <morgan@cal.com>
4 participants
What does this PR do?
Mandatory Tasks (DO NOT REMOVE)
How should this be tested?
Running the webapp and the examples app that we have in packages/platform/examples/base should be good enough