-
Notifications
You must be signed in to change notification settings - Fork 63
Rename NoticeBar and MigrationNotice components #894
Conversation
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.
This one looks good. LGTM!
@obulat do you know if there's a way to see the translation banner locally? I'm not confident it isn't broken (unrelated to this PR) because I can't view it anywhere. |
The easiest way to check locally would be to make this function in composable return false: openverse-frontend/src/composables/use-i18n-sync.js Lines 69 to 74 in 8e50891
|
@obulat ah, I forgot about that, thanks! To ask a different way: why doesn't the bar show up on http://10.1.84.207:8443/ru, is that because the translation % comes in from the wp theme? |
Yes, we need a setLocale message to show the translation banner. I guess we should change this |
Oh it also doesn't show locally otherwise anyway. |
@sarayourfriend I think that's only part of it because it doesn't display on http://localhost:8443/ru/search locally either, which doesn't use the |
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.
This LGTM. I also can't test the "normal" flow of the translation banner but the change is pretty low risk 🙂 Seems more like it would probably break on the .org theme side than anything.
Is that something we could change how it works using the path-based locale forwarding?
Fixes
Fixes #495 by @obulat
Description
This PR renames the NoticBar and MigrationNotice components, adding the
V
prefix.Testing Instructions
Checklist
Update index.md
).main
) or a parent feature branch.Developer Certificate of Origin
Developer Certificate of Origin