-
Notifications
You must be signed in to change notification settings - Fork 54
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
Replace/admin interface #637
Conversation
169b685
to
49757a5
Compare
ea0e703
to
94e4d43
Compare
Gives us smaller filesizes.
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.
Took the liberty to:
- Add missing translations
- Change FAQ images to webp, which gives us a smaller filesize
Co-authored-by: Vicente Canales <1157901+vcanales@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.
Looking good! Just a few nitpicks regarding eslint
rule deactivation
I believe all of the feedback has been addressed with one exception (that I'm not sure I understand). There was some great reviewing in here! Maybe ready for another look? |
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.
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 for the update! I have made additional suggestions.
Another thing that bothers me is that the translation files don't seem to be loading.
This plugin is already translated into several locales. And if I switch the site language to that locale, the script with the handle create-block-theme-app-js-translations
should be enqueued, but it isn't.
For example, it is 100% translated in the Japanese locale, and the editor sidebar is fully translated. I was hoping that this new admin page would translate at least some of it, but it doesn't translate at all.
Will this not be translated unless the plugin is released and reflected in GlotPress?
Co-authored-by: Aki Hamano <54422211+t-hamano@users.noreply.github.com>
Co-authored-by: Aki Hamano <54422211+t-hamano@users.noreply.github.com>
Co-authored-by: Aki Hamano <54422211+t-hamano@users.noreply.github.com>
Co-authored-by: Vicente Canales <1157901+vcanales@users.noreply.github.com>
Latest round of adjustments are in. Ready for another review I think! |
I've no idea why the translations wouldn't be working as expected. That might be related... Should we release this change and see? |
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.
👍
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 🚀
I think if we address the comment left by @vcanales, we can ship this PR.
Another thing that bothers me is that the translation files don't seem to be loading.
I checked the return value of wp_set_script_translations()
and it is true
, so there should be no problem with the implementation. Let's see if the translations will be reflected when this version is released and localized.
Co-authored-by: Vicente Canales <1157901+vcanales@users.noreply.github.com>
Replace the wp-admin interface with a React app that does the following: