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.

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

Fix UI issues with Wagtail 4.0 release #558

Closed
thibaudcolas opened this issue Apr 27, 2022 · 11 comments
Closed

Fix UI issues with Wagtail 4.0 release #558

thibaudcolas opened this issue Apr 27, 2022 · 11 comments

Comments

@thibaudcolas
Copy link
Member

The Wagtail 3.0 first release candidate is out. There are large UI changes in this release, for which we have reviewed expected breakage in third-party UI customisations.

This is beyond what we do with our normal breaking changes policy, since the majority of those changes are on parts of Wagtail that haven’t been publicly supported / documented in any way. To make sure this goes smoothly anyway, I’m here to provide an advance notice of what we’re aware of with this specific package 🙂

In the case of wagtail-localize there are three changes we’re expecting to require rework for Wagtail 3.0.

Uppercase text

The majority of the Wagtail admin UI no longer uses uppercase text, to improve readability. The exception is the page status (live, draft, etc.). Suggested actions:

  • Remove all usage of uppercase text in the CMS, except for page status.
  • Aside from CSS, the utility classes u-text-transform-uppercase and label-uppercase no longer exist and shouldn’t be used anymore.

Here are matches in this repository, which will likely need removing:

wagtail/wagtail/wagtail-localize/wagtail_localize/static_src/common/components/Section/index.tsx
10:    text-transform: uppercase;

wagtail/wagtail/wagtail-localize/wagtail_localize/static_src/editor/components/TranslationEditor/segments.tsx
325:    text-transform: uppercase;

Header template reuse

We’ve changed headers across most of the CMS. We are expecting those changes to be cosmetic only and not affect what customisations the header supports, but this is nonetheless worth reviewing manually.

wagtail/wagtail/wagtail-localize/wagtail_localize/locales/templates/wagtaillocales/confirm_delete.html
5:    {% include "wagtailadmin/shared/header.html" with title=view.page_title subtitle=view.get_page_subtitle icon=view.header_icon %}

wagtail/wagtail/wagtail-localize/wagtail_localize/locales/templates/wagtaillocales/edit.html
7:    {% include "wagtailadmin/shared/header.html" with title=view.page_title subtitle=view.get_page_subtitle icon=view.header_icon %}

wagtail/wagtail/wagtail-localize/wagtail_localize/templates/wagtail_localize/admin/submit_translation.html
6:    {% include "wagtailadmin/shared/header.html" with title=view.get_title subtitle=view.get_subtitle icon="doc-empty-inverse" %}

wagtail/wagtail/wagtail-localize/wagtail_localize/templates/wagtail_localize/admin/update_translations.html
7:    {% include "wagtailadmin/shared/header.html" with title=view.get_title subtitle=view.get_subtitle icon="doc-empty-inverse" %}

wagtail/wagtail/wagtail-localize-git/wagtail_localize_git/templates/wagtail_localize_git/dashboard.html
6:    {% include "wagtailadmin/shared/header.html" with title="Pontoon" icon="doc-empty-inverse" %}

Core templates reuse

It’s very hard to assess whether those customisations will suffer from any of our internal changes. This is worth reviewing manually.

wagtail/wagtail/wagtail-localize/wagtail_localize/locales/templates/wagtaillocales/edit.html
1:{% extends "wagtailadmin/base.html" %}
7:    {% include "wagtailadmin/shared/header.html" with title=view.page_title subtitle=view.get_page_subtitle icon=view.header_icon %}
32:                        {% include "wagtailadmin/shared/field_as_li.html" %}

wagtail/wagtail/wagtail-localize/wagtail_localize/locales/templates/wagtaillocales/index.html
1:{% extends "wagtailadmin/generic/index.html" %}

wagtail/wagtail/wagtail-localize/wagtail_localize/locales/templates/wagtaillocales/confirm_delete.html
1:{% extends "wagtailadmin/generic/confirm_delete.html" %}
5:    {% include "wagtailadmin/shared/header.html" with title=view.page_title subtitle=view.get_page_subtitle icon=view.header_icon %}

wagtail/wagtail/wagtail-localize/wagtail_localize/templates/wagtail_localize/admin/submit_translation.html
1:{% extends "wagtailadmin/base.html" %}
6:    {% include "wagtailadmin/shared/header.html" with title=view.get_title subtitle=view.get_subtitle icon="doc-empty-inverse" %}
21:                        {% include "wagtailadmin/shared/field_as_li.html" %}

wagtail/wagtail/wagtail-localize/wagtail_localize/templates/wagtail_localize/admin/edit_translation.html
1:{% extends "wagtailadmin/generic/edit.html" %}
16:    {% include "wagtailadmin/pages/_editor_css.html" %}
20:    {% include "wagtailadmin/pages/_editor_js.html" %}

wagtail/wagtail/wagtail-localize/wagtail_localize/templates/wagtail_localize/admin/_components.html
9:                {% include "wagtailadmin/shared/field_as_li.html" with li_classes="component-form__fieldname-"|add:field.name %}

wagtail/wagtail/wagtail-localize/wagtail_localize/templates/wagtail_localize/admin/update_translations.html
2:{% extends "wagtailadmin/base.html" %}
7:    {% include "wagtailadmin/shared/header.html" with title=view.get_title subtitle=view.get_subtitle icon="doc-empty-inverse" %}
53:                            {% include "wagtailadmin/shared/field_as_li.html" %}

wagtail/wagtail/wagtail-localize/wagtail_localize/templates/wagtail_localize/admin/translations_report.html
1:{% extends 'wagtailadmin/reports/base_report.html' %}

wagtail/wagtail/wagtail-localize/wagtail_localize/templates/wagtail_localize/admin/edit_translatable_alias.html
1:{% extends "wagtailadmin/pages/edit.html" %}

I hope this all makes sense. We’ve made a lot of other styling and template changes that are hard to track down, and for which it’s unclear whether any breakage might be expected or not. As you go through the Wagtail 3.0 compatibility work, please let me know if there are other compatibility issues you come across so we can consider those customisations in Wagtail development in the future, and let others know about those breakages.

@enzedonline
Copy link
Contributor

FYI - I ran into this trying to edit translated pages on 3.0rc2:

Error during template rendering
In template C:\...\.venv\lib\site-packages\wagtail_localize\templates\wagtail_localize\admin\edit_translation.html, error at line 6

string indices must be integers

1	{% extends "wagtailadmin/generic/edit.html" %}
2	{% load i18n wagtailadmin_tags %}
3	
4	{% block bodyclass %}page-editor{% endblock %}
5	
6	{% block titletag %}{% blocktrans with instance=translation.source.as_instance locale=translation.target_locale %}Translation of {{ instance }} into {{ locale }}{% endblocktrans %}{% endblock %}

The error appears to be in {% blocktrans with instance=translation.source.as_instance locale=translation.target_locale %}

@zerolab
Copy link
Collaborator

zerolab commented Apr 28, 2022

Hey @enzedonline,

we do not yet have official 3.0 support. That is pending.
If you have the time, a PR for this would be most welcome

@janbaykara
Copy link
Contributor

Should this issue be closed following the 1.2 release which supports Wagtail 3.0? https://github.com/wagtail/wagtail-localize/releases/tag/v1.2

@zerolab
Copy link
Collaborator

zerolab commented Aug 17, 2022

#560 only fixed the header styling. I think we should reporpuse this for the Wagtail 4.0 UI compatibility as 4.0 finishes the UI changes started in 3.0

@zerolab zerolab changed the title Expected UI issues with Wagtail 3.0 release Fix UI issues with Wagtail 4.0 release Aug 17, 2022
@janbaykara
Copy link
Contributor

Sounds good. Happy to volunteer on this as we are launching a project soon that would ideally lean on Wagtail 4.0 + wagtail-localize. I'm currently looking in to potential issues.

@zerolab
Copy link
Collaborator

zerolab commented Aug 17, 2022

That would be great.

The biggest thing will be header markup and overall styling. At the moment we still retain the teal colours whereas the 4.0 UI is much lighter

@janbaykara
Copy link
Contributor

janbaykara commented Aug 18, 2022

Just making notes here of things to look into:

  • Need to use the get_revision_model method in the migrations
  • CSS variables need updating (--w-)

@janbaykara
Copy link
Contributor

janbaykara commented Aug 18, 2022

Not to question fundamentals too much but is there a particular reason why the whole of the translation editor is implemented in React?

Wondering if it wouldn't be easier to maintain alignment with the Wagtail admin if it this package leant on the same header templates. For example, the new page editor's header has metadata stuff going on etc that would be nice not to lose.

I'm just getting stuck into this though, there might be an obvious reason.

@zerolab
Copy link
Collaborator

zerolab commented Aug 18, 2022

Not to question fundamentals too much but is there a particular reason why the translation editor is implemented in React?

This was one of the first questions I had when I started working on localize. The short answer is there was supposed to be a lot more functionality that lent itself nicely with React. However, the scope and priorities shifted since.

I would be all up for dropping the React requirement as it would also make fixing things like #508 or #364 faster too, but I don't have enough dedicated time to make it happen.

@janbaykara
Copy link
Contributor

janbaykara commented Aug 18, 2022

OK, so I think dropping React makes sense given the context. The impending introduction of StimulusJS into Wagtail means decent interactivity could be layered on top at a later date if required.

For the sake of speed and compatibility with 4.0, I'll submit a PR that brings in just enough of the header template that's required, but keeps the majority of the editor in React.

@zerolab
Copy link
Collaborator

zerolab commented Sep 23, 2022

this was fixed in #602 and the follow up #613
It is far from feeling native, but it is much closer to the Wagtail 4.0 interface than before

@zerolab zerolab closed this as completed Sep 23, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants