-
Notifications
You must be signed in to change notification settings - Fork 63
Conversation
Wouldn't having translations in the URL allow entering invalid combinations like https://ru.wordpress.org/openverse/es? |
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.
Works great, and will prevent the problems with mismatch with the outer frame we sometimes have (Openverse in French with outer frame in English).
I've scanned the repo for paths, and there is one place where we need to change path to localePath
:
:to="{ path: `/search/${key}`, query: $route.query }" |
I don't think this change should affect the urlChange
message exchange with the outer frame in sendWindowMessage
, but it would be nice to check.
I guess we could add a redirect to URLs like this. Is it possible to detect the mismatch between the |
@dhruvkb it what scenario would a url like ru.wordpress.org/openverse/es be produced? What would be adding the |
It wouldn't be occurring naturally, that's just my imagination of someone trying to mess with the system by intentionally entering such a mismatched, incorrect URL into the address bar. |
Got it! In this case, I think it would just 404 and that's okay. |
Thanks @obulat I also fixed the 'back to search results' links; I would've missed that without your suggestion! |
Oh also @dhruvkb, only folks using the non-english site who would know to inspect the iframe would even think to look at the url there, notice we're using a base path for locales, and then try to manipulate, where it would only effect their local browsing experience. |
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.
Just a couple of things/minor logic bugs.
Otherwise this LGTM. We'll still need one more fix to the homepage to fully fix the internationalization of it as #765 still needs to be addressed.
Co-authored-by: sarayourfriend <24264157+sarayourfriend@users.noreply.github.com>
Co-authored-by: sarayourfriend <24264157+sarayourfriend@users.noreply.github.com>
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.
LGTM! Thanks for the fixes 🎉
* main: (73 commits) Make audio/image pages without ids show a 404 (#768) Fix logo button paddings and simplify implementation (#767) Fix global audio rtl close placement (#780) Check for `null` localStorage explicitly (#763) Truncate global audio text to two lines (#773) New image details page (#682) Switch to path-based i18n routing (#701) Enable source maps in production (#755) Remove Jamendo and Wikimedia Commons from audio meta sources (#747) Use jed1x json format to correctly handle pluralization (#753) Fix logo color on error page layout (#752) Add homepage content switcher for mobile screens (#716) Add inline-start border to filters on desktop (#748) Fix header items not fitting in (#718) Expose `close` to popover content via slot props (#736) Truncate row layout audio titles (#735) Stop blocking on analytics requests (#715) Style links globally (#727) Refactor the usage of i18n result count computation (#707) Use `VPopover` for the content report form (#719) ...
Fixes
Fixes #700 by @zackkrida
Description
This PR switches Openverse to use the url path instead of a cookie for setting translations. This has a few benefits:
To use this PR, we need an accompanying PR in the Openverse WordPress theme that prefixes the iframe url with the correct locale string.
Testing Instructions
localhost:8443/es
,localhost:8443/ru
and so on.Checklist
Update index.md
).main
) or a parent feature branch.Developer Certificate of Origin
Developer Certificate of Origin