Skip to content
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.

Sign up for GitHub

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

Jump to bottom

[RFC 54] Locale management UI #6245

Merged
merged 3 commits into from
Sep 21, 2020
Merged

[RFC 54] Locale management UI #6245

merged 3 commits into from
Sep 21, 2020

Conversation

kaedroho
Copy link
Contributor

@kaedroho kaedroho commented Jul 20, 2020 •
edited by zerolab
Loading

Builds on #6220

@squash-labs
Copy link

squash-labs bot commented Jul 20, 2020

Manage this branch in Squash

Test this branch here: https://kaedrohoi18n-localeui-glnvg.squash.io

@kaedroho kaedroho force-pushed the i18n-localeui branch 3 times, most recently from 4314e6c to 664a8d8 Compare July 22, 2020 14:47
@kaedroho kaedroho force-pushed the i18n-localeui branch 2 times, most recently from 2b9d02d to 6b488c2 Compare July 30, 2020 07:18
@kaedroho kaedroho mentioned this pull request Jul 30, 2020
Closed
16 tasks
@kaedroho kaedroho force-pushed the i18n-localeui branch 2 times, most recently from c0c7a79 to c2632e9 Compare August 6, 2020 12:33
@gasman gasman added this to the 2.11 milestone Sep 3, 2020
@kaedroho kaedroho mentioned this pull request Sep 9, 2020
Closed
16 tasks
@kaedroho kaedroho force-pushed the i18n-localeui branch 2 times, most recently from a9884a3 to 86c47fa Compare September 15, 2020 14:46
@kaedroho kaedroho force-pushed the i18n-localeui branch 4 times, most recently from 15b4a96 to 3c72803 Compare September 17, 2020 12:10
@kaedroho kaedroho marked this pull request as ready for review September 17, 2020 12:14
@gasman gasman added the component:i18n i18n for content created in Wagtail, not the admin UI itself label Sep 17, 2020
gasman
gasman requested changes Sep 17, 2020
View reviewed changes
Copy link
Collaborator

@gasman gasman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks pretty good - just a minor quibble about the use of _editor_js.html (and imports need fixing to placate isort).

wagtail/locales/templates/wagtaillocales/create.html Outdated

{% block extra_js %}
{{ block.super }}
{% include "wagtailadmin/pages/_editor_js.html" %}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nope, removed!

@gasman
Copy link
Collaborator

gasman commented Sep 18, 2020

Looks like wagtail.locales.utils.get_locale_usage isn't playing well with the one-off invalid model created in TestSystemChecks - once that test has run, the model is returned in get_translatable_models, causing the subsequent query to fail as it doesn't have a table.

I'm thinking the best way around that is going to be either: find a way to un-register the invalid model after use (which looks a bit messy, involving fiddling around in internal properties of AppConfig and clearing the lru_cache on apps.get_models), or tweaking the test so that it temporarily modifies a real model from testapp.models rather than creating a new one.

@kaedroho
Copy link
Contributor Author

@gasman thanks! That issue is fixed now

@gasman gasman merged commit 208f756 into wagtail:master Sep 21, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gasman gasman gasman requested changes

Assignees
No one assigned
Labels
component:i18n i18n for content created in Wagtail, not the admin UI itself
Projects
None yet
Milestone
2.11
Development

Successfully merging this pull request may close these issues.
2 participants
@kaedroho @gasman