UI architecture technical debt #7689
Replies: 26 comments 55 replies
-
|
@thibaudcolas mentions "Too much nesting" as one of the root causes. I like to add this is not just nesting in CSS, it also stems from nesting of tempalte tags and extending templates. For example: I think the initial effort should be to normalise the templates, and than refoactor the classes and their corresponding SCSS. Having digged though the scss and the templates, it is going to be a quite an effort to sort things out, but it is about time to make a start. The milestone "Some day" is a bit concerning. As the CMS grows, this issue will continue to get more complicated every month. |
Beta Was this translation helpful? Give feedback.
-
|
I forgot to meantion: |
Beta Was this translation helpful? Give feedback.
-
|
Thank you for your thoughts @Meteor0id. You're very right to mention there are maintainability issues with the HTML too. One is what you mention. Another is overuse of For the milestone, we mark things as "some-day" by default, they only get to another milestone if they are scheduled as part of a release. The things tracked on this issue are separate individual problems, so they are unlikely to be worked on as a single item, and to be considered fixed by any single PR. Lots of people in the core dev team and beyond are aware of those issues, and of their impact on long-term maintainability of the codebase, so we're trying to tackle them nonetheless. Based on recent discussions among core devs, I have renamed this issue to be about "UI architecture" rather than CSS only (if I had more time, I'd also add HTML/JS items to the problems/caveats/solutions above). I think now would also be a good time to share notes from the 2018 State of Wagtail meeting about front-end maintainability, which is very relevant to this issue, and shows where the core team is at. I wanted to make a blog post out of those but never got around to doing it 😅. This "State of Wagtail" meeting was held at Wagtail Space 2018, with part of the core dev team present, and open to the public. State of Wagtail 2018 – Front-end maintainability
The problem
Examples of those problems:
SolutionsPattern library
Improvements to the existing tooling
Tests
Targeted refactorings (thankless pit of despair)
(more front-end developers)
Impact / effort matrix of the above
Potential work to trial this onAccessibility improvementsChoosers redesign and rebuild
New login flow(front-end extensibility by third-party packages)?
|
Beta Was this translation helpful? Give feedback.
-
|
Could be worth having a look at for the e2e tests http://cypress.io/ :) |
Beta Was this translation helpful? Give feedback.
-
|
Just wanted to add also that it appears that the version of Eslint we are using (~2.9.0) is now quite old, with v5 being released recently. I know this ticket is mostly about scss/css but just FYI. |
Beta Was this translation helpful? Give feedback.
-
|
FYI we’ve discussed this in today’s core team meeting, specifically from the perspective of legacy (vintage!) JS libraries (raised by @kaedroho). The TL;DR; from this discussion is that we intend to move away from jQuery (towards vanilla JS), jQuery UI & Bootstrap (towards more purpose-specific libraries). And of course to Webpack as the build tool for all JS & CSS of the Wagtail admin. |
Beta Was this translation helpful? Give feedback.
-
|
I’ve recently been considering which parts of this were still relevant, and whether to make a "2020" update to this all – the answer is that pretty much all of this still is relevant, and it’s felt quite heavily when doing major changes like those necessary for accessibility (#4199), or icons (#6107), which took many months’ more effort than if Wagtail’s front-end was better architected. We essentially waste a lot of time updating and testing patterns copy-pasted inconsistently across a big app, rather than being able to change components in isolation. I’ll focus the rest of the update on solutions rather than problems though. Changes since the last update
What to do nextThe overall direction I would suggest to get out of this mess is to refactor the whole of Wagtail’s UI to be powered by a living pattern library, as suggested in #3804 (comment) and initially in #382. Now working at Torchbox, I have a pretty clear sense of what this would look like: using https://github.com/torchbox/django-pattern-library, in conjunction with Storybook (https://github.com/torchbox/storybook-django), so we can have a living pattern library for both Django Templates, and React components. Both of those projects are relatively recent, but django-pattern-library has been in use for years internally at Torchbox, and Storybook itself has been around for years too. The advantages of this approach are that:
With the Storybook version of this pattern library, we’d be able to leverage the Storybook ecosystem, which has well established tools for UI library smoke testing, visual regression testing, and accessibility checks. As well as integrations with design tools (Sketch, InVision, Zeplin). This could be a way for us to modernize Wagtail’s approach to design, and make it more open to third-party contributions (see https://github.com/wagtail/rfcs/blob/master/text/037-accessibility-support.md#3-make-universal-design-and-accessibility-part-of-wagtails-design-and-development-process). What would this look like practically?Components would be defined as folders, with all of the components’s code co-located: The template partial, React component, stylesheet, test data, vanilla JS widget, per-component documentation, would all live in the same folder. For a Django templates integration, components that would make use of it could define their own template tag like our new Here is what component reuse would look like: {# Django templates partials #}
{% include "ui/button/button.html" with label="Get started" variant="outline" color="primary" href="/" %}
{# Django templates tags #}
{% button label="Get started" variant="outline" color="primary" href="/" %}
{# React #}
<Button label="Get started" variant="outline" color="primary" href="/" />
{# Jinja macro #}
{{ button(
label="Get started",
variant="outline",
color="primary",
href="/",
) }}
{# Django templates partials #}
{% include "ui/banner/split_banner.html" with title="Stories from our Salesforce migration" description="Whisperly Ltd" image=image %}
{# Django templates tags #}
{% split_banner title="Stories from our Salesforce migration" description="Whisperly Ltd" image=image %}
{# React #}
<SplitBanner title="Stories from our Salesforce migration" image={image}>
<p>Whisperly Ltd</p>
<Button href="/">Get Started</Button>
</SplitBanner>
{# Jinja macro #}
{{ split_banner(
title="Stories from our Salesforce migration",
description="Whisperly Ltd",
image=image,
) }}
{# Jinja macro with children #}
{% call split_banner(title='Hello World') %}
<p>Whisperly Ltd</p>
{{ button(label="Get started", href="/") }}
{% endcall %} |
Beta Was this translation helpful? Give feedback.
-
|
The custom tags remind me of Slippers, which is really a joy to work with. |
Beta Was this translation helpful? Give feedback.
-
|
We’ve discussed the UI component library proposal above in today’s accessibility team meeting, there was some consensus that this felt like a good way forward. I think I’ll open an RFC to describe this further and more formally ask for feedback. |
Beta Was this translation helpful? Give feedback.
-
|
@thibaudcolas for React components, just wondering if you've thought much about how we could export these for third party apps to reuse? (such as wagtail-localize, wagtail-review) |
Beta Was this translation helpful? Give feedback.
-
|
We’ve thought about this in the past but never made any big steps towards that goal. The options are:
|
Beta Was this translation helpful? Give feedback.
-
|
Once we've ended IE11 support, we would be able to use ES6 modules in Wagtail. I'm wondering if that makes this problem easier to solve? I'm thinking that Wagtail could provide a UI component library on an importable path. This UI library would have a stable, documented API and Wagtail would use this library internally so it should already be loaded in the browser. For example, a third-party app implementor could import the import { Icon } from '/static/wagtail/components.js';Do you think that could work? |
Beta Was this translation helpful? Give feedback.
-
|
Hmm good question, that definitely sounds worth trying :) I don’t know enough about ES modules bundling to have an informed opinion. This sounds similar to the "global variables on the page" option, with the big benefit that you’d be able to dynamically load only the components you need. I imagine this would need to use the dynamic |
Beta Was this translation helpful? Give feedback.
-
|
Now that Wagtail uses GitHub Discussions, I thought this would be a good fit for this… discussion, since it’s not likely to be "fixed" by any one change. |
Beta Was this translation helpful? Give feedback.
-
|
Now, the 2021 update 😄 . I have much more time than ever before to help tackle the problems outlined here, and I believe those issues would be a good fit for a dedicated group of contributors to form a Wagtail sub team. This issue is likely the best place to look for people who might be interested in joining such a team, and also to get feedback on what we might tackle. Any feedback, comments, criticism very welcome. Feel free to reply here, or follow up in Slack in the What we’d doI see the team’s remit as:
The team would ideally be cross-disciplinary, and have some people committed to short-term sustained involvement (say a day per week), with a bigger group of people at the periphery interested in contributing to UI decisions via GitHub Discussions and Slack. The closest to this is the accessibility team, which currently has 5 people and uses Proposed backlogI have started working on a backlog of things I think would have the highest impact as long-term UI improvements for Wagtail, via a UI team project board on GitHub. |
Beta Was this translation helpful? Give feedback.
-
|
I'd be excited to be on this subteam and to resume contributing to refactoring. And I'm very in favor of setting up pattern libraries! |
Beta Was this translation helpful? Give feedback.
-
|
I’d also strongly consider putting my hat in the ring here, assuming I can find some time to set aside here and there! |
Beta Was this translation helpful? Give feedback.
-
|
Hello Thibaud, great that you are taking the lead. 🥇 Yes, let's finish the SVG icons #4821. I really underestimated the effort involved. Storybook and Django Storybook seem useful. It will help develop Wagtails frontend without setting up a website with various content types. It might help with frontend testing too. Storybook is already in Wagtails node_modules. Django Storybook is not used yet. Maybe we should make a POC to illustrate the benefits? Refactoring code in Wagtail is a lot of work. Documenting the desired patterns will help contributors to do the right thing. I'd like to contribute where I can. |
Beta Was this translation helpful? Give feedback.
-
|
🤘 glad to have you here @allcaps 😌 The prototype seems like a great idea, I’ll get on with it. |
Beta Was this translation helpful? Give feedback.
-
|
Howdy team! I wanted to bring up the idea of namespacing our CSS classes within the admin. With generic class names like Here's an example of the conflict in action; the Wagtail admin's
If you look at the So maybe we can? Thanks for any thoughts y'all have. |
Beta Was this translation helpful? Give feedback.
-
|
Yep, I think we can! I’m not entirely sold on prefixing everything, as that would increase the verbosity of class names a fair bit, but there are other options:
|
Beta Was this translation helpful? Give feedback.
-
|
Hey, I think having 'root' component classes is helpful (for example the root of the dropdown could be wg-dropdown or wag-dropdown) and then classes within that container just follow a more basic style (e.g. dropdown-item). This way our styles can be scoped |
Beta Was this translation helpful? Give feedback.
-
|
@thibaudcolas I would like to put my hand up to help on this team. |
Beta Was this translation helpful? Give feedback.
-
|
@thibaudcolas I would be happy to help as well. Cf https://wagtailcms.slack.com/archives/C014L7KJH3N/p1637142176085700 |
Beta Was this translation helpful? Give feedback.
-
|
🤘 lovely to see so much interest in this! |
Beta Was this translation helpful? Give feedback.
-
|
I would like to see if we can leverage DOM Events more in our Wagtail components, either React ones or not, this provides an elegant way to 'hook' into behaviour and even modify things if needed. For example - for inline panel (with re-ordering), you either have to basically copy/paste the entire inline panel init code OR add event listeners to every single up/down/delete item along with to the 'add' button which will then register new event listeners to the newly created up/down/delete items. What if we just fired off a custom event for these in the core code for this component for the move and remove actions. This may be out of scope of this work but we could leverage this approach to provide a way to add custom behaviour on the frontend without having to always resort to building full custom versions of elements. https://developer.mozilla.org/en-US/docs/Web/Events/Creating_and_triggering_events |
Beta Was this translation helpful? Give feedback.
-
|
I like this very much. I have a usecase where I need the page ID value returned by a |
Beta Was this translation helpful? Give feedback.
-
|
thanks for the input @Jean-Zombie - glad you got the solution working based on our chats in Slack. I'm thinking we should ensure that chooser panel and things like it should at minimum trigger a This would also be good on the hidden inputs for inline panel order/deleted values. |
Beta Was this translation helpful? Give feedback.
-
|
Django 4.1 will use pure JS events (not jQuery events). Good to use this documentation and approach as a reference. Names
|
Beta Was this translation helpful? Give feedback.
-
|
Added an issue for the events (InlinePanel) #9105 |
Beta Was this translation helpful? Give feedback.
-
|
talking about this, it been about 3 weeks or so, investigating to provide a clean PR to reduce the over weights of static files, I realized that the main issue here is that elements are not in the same DOM ordering path, therefore it is causing Core.CSS and Core.js and other statics to be large as it is without necessity for that weight, for example: |
Beta Was this translation helpful? Give feedback.
-
|
another example: if I want to put these radio buttons beside each others, it is kinda impossible |
Beta Was this translation helpful? Give feedback.
-
|
it comes from: |
Beta Was this translation helpful? Give feedback.
-
Lightweight Frontend Framework Investigation@thibaudcolas tasked me with reviewing some of the lightweight frameworks out there. With the goal to see what may suitable for the middle ground of interactivity where React may not be an ideal option. We want to make it easy for those building Wagtail and building with Wagtail to think in HTML first (leveraging Django) but make a great experience for those editing width Wagtail. The desire is to be able to build frontend interactivity while weaning off the reliance on jQuery but not having a mess of inconsistent approaches of lower level DOM traversal/mutations scattered around the code. Links
Additional Details
More code examples
|
Beta Was this translation helpful? Give feedback.
-
|
Relevant article in relation to 'low JS' solutions. |
Beta Was this translation helpful? Give feedback.
-
|
Hi all, I have put together a full end to end draft of how Stimulus could work in Wagtail. https://github.com/lb-/wagtail/pull/4/files
Note: This has not been agreed upon at the core team level, but just adding here for transparency and feedback. @janbaykara @timonweb @conatus @easherma-truth & @tbrlpld - you all were pretty vocal a few months ago for the early discussions about a frontend lightweight framework so we would love to get your feedback. Note: The reasoning of leaning towards Stimulus can be found in the write up at the top of this thread's discussion. |
Beta Was this translation helpful? Give feedback.
-
|
Hi @lb-. Not much to add at this point, I think you've done a great job of covering things here. |
Beta Was this translation helpful? Give feedback.
-
|
RFC 78 created wagtail/rfcs#78 - it would be great to the community feedback on this, even if it is a comment with some buy in from those who have posted to this thread. The RFC contains full working code, along with all the feedback received along the way in this and other threads. If anyone really sees a major problem with Stimulus or this general direction in Wagtail - please take the time to comment on the RFC with your reasoning. While I would love to see this in the main code I would hate it if we have a blindspot for something critical that is not addressed. |
Beta Was this translation helpful? Give feedback.
-
|
Wagtail 4.2 introduces Stimulus for several JS driven widgets. Thanks to all those who have contributed to the discussion for this. https://docs.wagtail.org/en/stable/releases/4.2.html#csp-compatibility-stimulus-adoption |
Beta Was this translation helpful? Give feedback.
-
|
adding just another Option (https://brython.info/static_doc/en/intro.html) to the stack, Brython is a browser based python script, it is converting 95% of front-end coding to Pythonic |
Beta Was this translation helpful? Give feedback.
-
|
I don’t think we could seriously consider things that are so niche. Wagtail has quite advanced UI needs so we have to bet on tried-and-tested approaches. |
Beta Was this translation helpful? Give feedback.
-
|
I agree, it been hours that I am working on it, and need lots of testings, once I done with it, I may share a blog somewhere (for pros and cons of it) |
Beta Was this translation helpful? Give feedback.
-
|
I did test it a bit more, it turns out that nothing can hurt you with brython, except the large size of it's static file (stdlib) which does every fancy things to memic the JS scripts by Python Language within the HTML templates..! |
Beta Was this translation helpful? Give feedback.
-
|
For people following this discussion – here is a prototype of automated visual regression testing for Wagtail’s UI, implemented with Percy: thibaudcolas/wagtail-fork#4 This has been proposed on and on over the years, and I even had made a prototype with BackstopJS (which I’d still recommend as a tool for some projects) back in 2018. Now that Wagtail has Storybook up and running, and more recently an E2E browser automation test suite, we’ve finally reached a point where adding visual regression testing can be done in a cost-effective way. I’d love to hear what people here think of this – maybe with high-level questions / comments in this thread, and review of our proposed setup with Percy in that prototype pull request. |
Beta Was this translation helpful? Give feedback.
-
|
Is this serendipity or was this spawned by my short show and tell session? |
Beta Was this translation helpful? Give feedback.
-
|
Some quick thoughts on StimulusJS, based on a very rapid run though the first part of the tutorial... Personally I feel that I'd be more at home building things in the vanilla JS way, hooking up event handler callbacks and things manually - the Looking from the angle of infrastructure code that needs to integrate with arbitrary component JS - e.g. inserting new InlinePanels and ensuring that any widgets within them get initialised, which would currently be done |
Beta Was this translation helpful? Give feedback.
-
|
https://github.com/lb-/wagtail/pull/4/files updated with the ability to create a controller with basic JS, no need to import a module, use build tooling or even understand class inheritance. Example code that would create a word count validation on any field (from the ABOUT.md in the PR), just add Adding a new controller - word count limiting (without a build system or even classes!)// myapp/static/js/word-limit-controller.js
const controller = {
STATIC: {
values: { max: { default: 10, type: Number } },
},
connect: function () {
const output = document.createElement("output");
output.setAttribute("name", "word-count");
output.setAttribute("for", this.element.id);
output.style.float = "right";
this.element.insertAdjacentElement("beforebegin", output);
this.output = output;
this.updateCount();
},
updateCount: function (event) {
const value = event ? event.target.value : this.element.value;
const words = (value || "").split(" ");
const current = words.length;
const max = this.maxValue;
this.output.textContent = `${current} / ${max} words.`;
if (current > max) {
words.splice(-1 * (current - max));
this.element.value = words.join(" ");
}
},
};
document.addEventListener(
"wagtail:stimulus-init",
({ detail: { createController, register } }) => {
register(["word-limit", createController(controller)]);
},
// important: stimulus-init may be called more than once, only run the registration once
{ once: true }
);# models.py
# https://docs.wagtail.org/en/stable/reference/pages/panels.html#fieldpanel
from django import forms
class BlogPage(Page):
# ...
content_panels = Page.content_panels + [
FieldPanel('subtitle', classname="full"),
FieldPanel(
'introduction',
classname="full",
widget=forms.TextInput(
attrs={
'data-controller': 'word-limit', # connect to the controller
'data-word-limit-max-value': '5', # set the word limit for this field
'data-action': 'word-limit#updateCount paste->word-limit#updateCount', # set when the count should be updated (e.g. could use blur instead of change/paste)
}
)
),
#...# wagtail_hooks.py
# https://docs.wagtail.org/en/stable/reference/hooks.html
from django.utils.html import format_html_join
from django.templatetags.static import static
from wagtail import hooks
@hooks.register('insert_editor_js')
def editor_js():
js_files = ['js/word-limit-controller.js',]
return format_html_join('\n', '<script src="{0}"></script>',
((static(filename),) for filename in js_files)
) |
Beta Was this translation helpful? Give feedback.
-
|
That's really smart! |
Beta Was this translation helpful? Give feedback.
-
|
Great one |
Beta Was this translation helpful? Give feedback.
-
|
Have done some experimenting with this today... if you wrap this into a custom widget class you can avoid the need for the from django import forms
class WordCountTextInput(forms.TextInput):
class Media:
js = ['js/word-limit-controller.js',]
def __init__(self, words=5, attrs=None):
default_attrs = {
'data-controller': 'word-limit', # connect to the controller
'data-word-limit-max-value': words, # set the word limit for this field
'data-action': 'word-limit#updateCount paste->word-limit#updateCount', # set when the count should be updated (e.g. could use blur instead of change/paste)
}
if attrs:
default_attrs.update(attrs)
super().__init__(default_attrs)at which point you can easily make it into a StreamField block: class WordCountTextBlock(TextBlock):
@cached_property
def field(self):
field_kwargs = {"widget": WordCountTextInput(words=5, attrs={"rows": self.rows})}
field_kwargs.update(self.field_options)
return forms.CharField(**field_kwargs)and the initialisation works exactly as hoped - the word count is in place on all newly added blocks. Very happy with this! |
Beta Was this translation helpful? Give feedback.
-
|
nice - this is epic. |
Beta Was this translation helpful? Give feedback.
-
Wagtail's UI codebase is in bad shape. This isn't really surprising – UI/CSS architecture in long-lived projects is a notoriously hard problem. It's not hopeless either. But there is work to do 🙂
I'd like to use this as a tracking issue to discuss the problems this is creating, their causes, potential solutions, and hopefully gather consensus. In my opinion, a good outcome of those conversations would be to agree on a prioritised list of actions to be taken, that can then be worked on by different contributors in separate PRs. Sweeping rewrites are not on the agenda.
Problems
Causes
This is a grab bag of code smells and high-level architectural issues.
_main-nav.scss, 479 lines).nav-main .footer-submenu a).page-editorlayout have been defined inside of it.Solutions
With #3773 as a foundation, I believe some of the above could easily be addressed by increasing what rules are enforced with tooling.
stylelintrules or plugins to enforce stricter guidelinesselector-no-qualifying-type.stylefmtorprettier.wagtailnpm package so the variables and components can be reused in plugins.Related issues, in no particular order
Specificity hacks make it hard to customise Wagtail's UI Reduce specificity of Hallo CSS icon classes #2600Beta Was this translation helpful? Give feedback.
All reactions