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

Implements the new StreamField based on react-streamfield. #4942

Open
wants to merge 38 commits into
base: master
from

Conversation

Projects
None yet
8 participants
@BertrandBordage
Copy link
Member

BertrandBordage commented Dec 5, 2018

This pull request is the final version of the first and main goal of the first Wagtail crowdfunding campaign.

Main features:

  • Brand new fancy design
  • Straightforward structure
  • Drag and drop to move around the blocks
  • Blocks use as much width as possible, for a better nesting
  • Duplication of blocks
  • Blocks can be made collapsible for complex data structures
  • Smaller add panels, perfect when using more than a dozen blocks
  • Better performance by several orders of magnitude (up to 1000× faster for loading the edit page!)
  • New support for min_num and max_num in ListBlock
  • Django formsets are no longer used to handle the form data. Previously, we had to convert from the database JSON to formsets, then parse hundreds/thousands of fields with formsets to convert data to JSON. Now, a single hidden JSON input stores on the page the data that will be stored to the database, with almost no need for conversion (Draftail is an exception).
  • Far better responsiveness

You can already test it by using wagtail-react-streamfield, a library that patches your existing Wagtail during the execution (a.k.a. monkey patching) to add this pull request.

As you can see, this pull request is fairly small given the amount of work required by it. This is because almost all the magic is happening in react-streamfield, a JavaScript library I developed for the occasion. As of now, I spent a total of 179 hours on the campaign work (excluding preparation of the campaign), in addition to 120 hours spent benevolently on Wagtail since January.

So yes, things got a little out of hand and took much longer than I planned. But I still hang on and am still planning to finish everything I promised I will do.

For the integration, I had to update Babel, React and related librairies because react-streamfield relies on the latest JavaScript syntaxes and technologies.

It’s fully working, but I’m still struggling with some last tests (after updating/adding/removing dozens of them). The remaining tests rely on the old template rendering of the StreamField and are tricky to update, I still have to spend 10-30 minutes on each test.

Here is what it looks like:
image

The InlinePanel PR will follow, it’s enough to review for now ;)

@thibaudcolas My proposition of paying you to review this work is still up! If you don’t have time, please let us know so someone else can do it.

@allcaps

This comment has been minimized.

Copy link
Contributor

allcaps commented Dec 7, 2018

Hi @BertrandBordage,

I did a test drive of wagtail-react-streamfield, with a production site and production data. It works great! Thank you for all the great work. Congratulations on getting this done the way you did it. Awesome.

I noticed layout changes of blocks when the collapsible layout is enabled. I really like the layout of the collapsible as it gives most out of every pixel on small view ports. The difference between the two is probably a well considered design choice. My gut feeling says that layout change isn't the best indicator for collapsible or not. This is a minor thing. I'm happy to make everything collapsible in my projects ;). Although, the non collapsible blocks do have small issues that the collapsible block doesn't have:

Top block is NOT collapsible rich text, bottom block is collapsible table. Not optimal use of space available.
screenshot 2018-12-06 at 22 53 33

I think this is a bug, the choosers should be visible:
screenshot 2018-12-06 at 22 57 46
screenshot 2018-12-06 at 22 58 01

Last and not least, the contrast of text and icons is too low. In chrome I ran lighthouse accessibility audit:
screenshot 2018-12-07 at 09 40 39

Let me know if you can use any help.

@BertrandBordage

This comment has been minimized.

Copy link
Member Author

BertrandBordage commented Dec 7, 2018

Note that with the latest changes, the duplicate icon changed, I updated the icon font to add a new icon I created with Inkscape, based on the existing document icon:
image

@BertrandBordage

This comment has been minimized.

Copy link
Member Author

BertrandBordage commented Dec 7, 2018

Thank you very much for the feedback @allcaps!
With the SIMPLE layout, as discussed in the RFC, the action buttons must be in a separate column, we cannot make them float: right because it will mess with the block content.

That being said, the idea to improve SIMPLE responsiveness that we already discussed is to make it switch to COLLAPSIBLE automatically. I tried to implement it a few weeks ago, but couldn’t get it working reliably across browsers, so I postponed it for now. I will try again, maybe not in this PR to avoid delaying.

About the choosers being truncated, normally they are horizontally scrollable, the scrollbar is not visible because you’re in Chrome responsive tools.

Nice catch on the color accessibility. I followed @benenright’s design suggestion for this color, and all that was before the accessibility rebuild of the admin interface, so we didn’t have color contrast accessibility in mind at that time.

@gasman gasman self-requested a review Dec 13, 2018

@BertrandBordage

This comment has been minimized.

Copy link
Member Author

BertrandBordage commented Jan 2, 2019

@gasman The tests are finally passing! 🎉 😄

I made this pull request as much backend as possible, so it doesn’t require an in-depth frontend review.
Almost all the frontend is done by react-streamfield, which can be modified/improved outside Wagtail, like all the “vendor” dependencies we’re using.

So @gasman, could you make a review of this please? Since @thibaudcolas doesn’t seem available for review and the PR is almost not frontend, I’d like to pay you/Torchbox for the review. You can test it easily with wagtail-react-streamfield, I kept it up to date with the PR.

Note that I tested everything I could automatically, both on react-streamfield and this PR. It’s hard to test everything automatically without introducing Selenium, and we surely don’t want to maintain that ☹️
And I manually tested everything for dozens and dozens of hours, I tried to cover all the possible scenarios. I discovered a lot of little undocumented and previously somewhat buggy features, like raising a validation error of a StructBlock directly, or specifying a default value for a StreamBlock containing a ListBlock of StructBlock where the children blocks already have default values. To put it short, I spent months in the land of edge cases…

Some of the latest design tweaks concern the block type buttons that are bigger, with uppercase text:
image

@allcaps Note that the blocks all switch to COLLAPSIBLE now when we’re on mobile 😉 It works like a charm, even for deeply nested blocks!

image
image

As a final note:

  • Some design decisions could probably be improved, like the panel to select a new block type. In my opinion, all that can be dealt with later, as every single feature is IMO already much better than on the existing StreamField.
  • The react-streamfield module is not documented, and I don’t plan to document it for now. It could be a very powerful library for other projects, so I might document it in the end, but for now I consider it as an outsourced private API of Wagtail. The list of examples serves as the main source for understanding features and manually testing everything.
  • The react-streamfield module has a great data structure for performance, but the code structure could be improved. I did everything I could to keep it as clean as possible, but we’re dealing with JS and React here. I wanted to be DRY, I wanted to have a great class-based approach that we could have in Python, but due to the limitations we have for performance through React and Redux, the code is still a little ugly.
  • The third-party apps defining blocks with custom JS will need to be upgraded for compatibility with this new StreamField.

To sum up

This PR is 100% functional as far as I know.
I would say that it’s considerably more mature than the state of Draftail when we merged it. We had to fix a ton of little issues afterwards (mostly due to the complexity of parsing HTML/copy pasting, etc). For this PR, I guess we might still have a few edge cases issues opened once in a while, but for almost all users, everything should work out of the box.

@jjanssen

This comment has been minimized.

Copy link
Member

jjanssen commented Jan 3, 2019

Hi @BertrandBordage

This is fantastic! 💯 🎉 In between some initial UX findings from a beta-phase we installed during development and this pull request there is not much to add from a functional perspective.

The initial response from some our QA’ers and content managers was a bit overwhelming in terms of what was changed but it was more a moment of adapting rather than re-exploring how it works.

Although we have a few UX related findings I would suggest to gather some feedback from the rest of the community as well (and its customers) and the UX/Design team whether we could and/or should apply these findings and suggestions.

Perhaps it would have a better place in a new improvement issue after the merge but I’ll leave that up to you.

Creating a new page
When creating a new blank page it initially was a bit confusing for some that these are actual button actions to add a new block in the content area. When there is already existing content in the streamfield there is no confusion on what this new interface does.

image

Suggestion: perhaps it could benefit from a help text when there is no content defined yet or maybe some additional copy in the button.

Spacing
During editing the plus icon in between the blocks is more clear to the eyes in a mobile / tablet state. Yet on desktop it took a bit of guessing what the plus on the left does.

Suggestion: perhaps place the plus in the center in between the blocks on desktop as well.

image

Consistency
When there is no title the name of the block is on the left, if there is a title available the name of the black is aligned to the right. When having everything collapsed on entry it is experienced that orientating on the rendered component is delayed due to this behaviour.

Suggestion: keep the name of the block on the left so the area of scanning the page is not disrupted.

@thibaudcolas

This comment has been minimized.

Copy link
Member

thibaudcolas commented Jan 4, 2019

Hey @BertrandBordage, congratulations on pulling this off 👏 it's impressive work, to say the least.

I've been on holidays all of december and will be back at work in mid-january. Then I'm happy to have a look at this, paid or not.

@thibaudcolas
Copy link
Member

thibaudcolas left a comment

Added a note about how react-streamfield is bundled & integrated. @BertrandBordage could you explain why you chose to have the new StreamField code as a separate library?

@@ -0,0 +1 @@
import '../../../../node_modules/react-streamfield/dist/react-streamfield.js';

This comment has been minimized.

@thibaudcolas

thibaudcolas Jan 4, 2019

Member

Judging by how this code is included, I understand that the compiled dist/react-streamfield.js file includes all of the library's dependencies directly bundled? If so, this means that React + Redux and all their related libraries will be imported twice, among other issues (lack of package management for transitive dependencies). It would be better if:

  • Libraries already used by Wagtail were marked as peer dependencies, so we are guaranteed things like React & Redux are only included once, with the same version everywhere.
  • Other libraries should be declared as dependencies in the package.json, so Wagtail's client-side build can leverage npm to manage their versions (de-duping, & transitive dependency updates without having to re-release react-streamfield every time.

I would also expect some or all of the code in https://github.com/noripyt/react-streamfield/blob/c8a42f7700724e603de494d2432e3e966c5c6ca6/src/index.js to be defined in Wagtail instead of the library. It's more common for libraries based on React components to expose the components for integration, or at least an initialisation function, so the dependencies of the library are more clearly defined. Imports should be free of side-effects.

This comment has been minimized.

@BertrandBordage

BertrandBordage Jan 8, 2019

Author Member

I fully agree with you, this is the ideal solution.
I originally wanted to do exactly this, as you can see on the initial commit: 884e363

Unfortunately, I couldn’t manage to update all other projects to Babel 7, I kept facing conflicts, even after twenty hours spent on it… So I decided to make it a fully external dependency instead.

I also agree that react-streamfield itself must only be made of components and not have this auto-loaded logic in its index.js. But that’s for a scenario where the components API would be stabilized and documented, which I do not plan to do in a near future. It’s out of scope of this work, as discussed in noripyt/react-streamfield#3. So for now, the API is just pure JSON. I’m perfectly aware that it’s not the ideal React approach, but it’s still a perfectly valid React use case.

This comment has been minimized.

@thibaudcolas

thibaudcolas Jan 21, 2019

Member

I don't think it's out of scope for this, it's a real maintainability problem if we can't understand what a library does by looking at how it is used in the Wagtail code. In particular there is no way to know what dependencies it has on the context it's called in.

I think any API will be better than no API, so we can at least understand the relationship between what's in the package and what's out of it. It's fine for the API of the StreamField component to stay JSON-based as it is, and for that JSON structure not to be fully documented / fixed, but the initialisation of the component needs to be in Wagtail rather than in the library. I would also rather have the process by which this saves data defined in Wagtail (say an onChange prop on the StreamField, that then serialises its value in an input).


For the package build, I'm sure I can help. I don't think we need to update the development dependencies of Wagtail to match those of react-streamfield, since the compilation will be done as part of that package's build process, but we need the package to use the versions of React & co (and ideally Redux & co) to match those of Wagtail, so most likely update Wagtail’s.

@import '../../../../node_modules/react-streamfield/src/index';


.children-container {

This comment has been minimized.

@thibaudcolas

thibaudcolas Jan 4, 2019

Member

@BertrandBordage could you explain how this code relates to the styles defined in react-streamfield/src/index?

This comment has been minimized.

@BertrandBordage

BertrandBordage Jan 8, 2019

Author Member

This code is specific to Wagtail. Wagtail has some global CSS that override react-streamfield’s assumptions on the base styling loaded in the page. This SCSS fixes conflicts with Wagtail’s CSS, and also adds some Wagtail-specific styling, like the help text that should be in blocks.

This comment has been minimized.

@thibaudcolas

thibaudcolas Jan 21, 2019

Member

I would really like all of those overrides not to be there, it's really unhealthy for a CSS codebase to start piling on "reset overrides" like these. It's also a concern because the selectors used here are very long, and highly dependent on the structure of the HTML, which is managed in a separate dependency and could change without those overrides being updated.

I'll try and propose a different approach for some of these.

@gasman

This comment has been minimized.

Copy link
Collaborator

gasman commented Jan 8, 2019

I've put up a copy of Bakerydemo with this PR applied, for testing: https://bakerydemo-reactstreamfield.herokuapp.com/admin/

@tmsndrs

This comment has been minimized.

Copy link
Collaborator

tmsndrs commented Jan 8, 2019

This is feeling really close now! Well done to everyone who's put the work in!

I wonder if a Gutenberg approach would work for the plus icons
https://wordpress.org/gutenberg/

I was also confused by the duplicate feature. Perhaps the icon could change on hover to something symbolising duplication with a more Wagtail themed tool tip coming in a bit quicker. @benenright any thoughts on the style of this?

@BertrandBordage

This comment has been minimized.

Copy link
Member Author

BertrandBordage commented Jan 8, 2019

@jjanssen Thanks for taking the time to give that detailed feedback 😃

Creating a new page: That’s right, it could be clearer, but I’m not really sure where to put the help because of the possible groups. A top right floating help text with a little question mark icon?

Spacing: I’m not sure what to choose, given that intermediate pluses take a lot of space, especially because we want them to be big enough for accessibility. I’ll put a survey below for people to vote for or against it.

Consistency: Well, the whole point of these collapsed titles is for users to quickly identify the blocks when collapsed. I’m not sure it’s that misleading to have the type that goes to the right, which is roughly where they go when SIMPLE layout is used.

@BertrandBordage

This comment has been minimized.

Copy link
Member Author

BertrandBordage commented Jan 8, 2019

EVERYONE:
On desktop, would you prefer to have full width centered pluses over the currently left guttered ones?
Vote 👎 to keep left guttered and 👍 for full width centered pluses.
Vote 😕 if you don’t care.

Current behaviour, left guttered pluses

image

Suggested behaviour by @jjanssen, full width centered pluses

image

@BertrandBordage

This comment has been minimized.

Copy link
Member Author

BertrandBordage commented Jan 8, 2019

@tmsndrs Interesting approach that Wordpress is taking! If you’re referring to the big squarish buttons with labels below, we already discussed that in the past, as it was Wagtail’s previous approach. The big downside is that your buttons are centered around icons, and most of the time we don’t have existing icons for project-specific blocks, and we don’t want to create specific icons just for one project. In our not-plugin-based case, this is not really relevant. Additionally, we ended up with issues with long labels, especially in some languages like German where you have long words.

What do you find unclear about this icon?
image
As far as I know, it’s the standard look for a copy/duplicate icon since at least 25 years, right?

@Adrian-Turjak

This comment has been minimized.

Copy link

Adrian-Turjak commented Jan 8, 2019

Spacing
During editing the plus icon in between the blocks is more clear to the eyes in a mobile / tablet state. Yet on desktop it took a bit of guessing what the plus on the left does.

Suggestion: perhaps place the plus in the center in between the blocks on desktop as well.

image

The benefit of this new StreamField is that it makes editing HUGE (and nested) pages much easier. The issue with making desktop look the same as mobile is that the added space between the blocks makes the overall size (vertically) bigger. On mobile that's fine, you'll scroll and you probably won't edit on there quite as much, but on desktop the added padding from having those pluses in the middle is frustrating.

What we can potentially do is have the mouse over effect on desktop for the plus be to increase the padding between blocks where the new block would go, thus giving some visual feedback about what the button might do, and making it clearer that it is interactive.

The UX isn't exactly that hard to get once you've used it once and anything to decrease vertical size is good.

@Adrian-Turjak

This comment has been minimized.

Copy link

Adrian-Turjak commented Jan 8, 2019

I'd also like to point to my issue: noripyt/wagtail-react-streamfield#31

Just because I feel like SIMPLE as the default layout kind of defeats a lot of the benefits this StreamField offers over the old one, and the current methods for controlling SIMPLE vs COLLAPSIBLE are lacking.

@tmsndrs

This comment has been minimized.

Copy link
Collaborator

tmsndrs commented Jan 11, 2019

@tmsndrs Interesting approach that Wordpress is taking! If you’re referring to the big squarish buttons with labels below, we already discussed that in the past, as it was Wagtail’s previous approach. The big downside is that your buttons are centered around icons, and most of the time we don’t have existing icons for project-specific blocks, and we don’t want to create specific icons just for one project. In our not-plugin-based case, this is not really relevant. Additionally, we ended up with issues with long labels, especially in some languages like German where you have long words.

What do you find unclear about this icon?
image
As far as I know, it’s the standard look for a copy/duplicate icon since at least 25 years, right?

I was actually just referring to the plus icon coming in on hover in Gutenburg. My personal preference would be for a smaller, centred plus symbol that is clickable from anywhere in the space between blocks. The one demoed above is too big and creates to much white space for me.

On the duplicate icon, I think I was thrown off by the block type sitting directly below it.

@tmsndrs

This comment has been minimized.

Copy link
Collaborator

tmsndrs commented Jan 11, 2019

What we can potentially do is have the mouse over effect on desktop for the plus be to increase the padding between blocks where the new block would go, thus giving some visual feedback about what the button might do, and making it clearer that it is interactive.

This 👍

@thibaudcolas

This comment has been minimized.

Copy link
Member

thibaudcolas commented Jan 18, 2019

I'm going to review this PR and react-streamfield for the front-end part, and have also deployed test environments of the bakerydemo to make it easier. They contain more advanced StreamField implementations compared to the vanilla bakerydemo (wagtail/bakerydemo#168). Feel free to use these if you want to try it out as well:

Methodology described here if anyone wants to make their own test environment with Heroku

@thibaudcolas
Copy link
Member

thibaudcolas left a comment

I have spent hours reviewing this new StreamField, code-wise and UX-wise, and I'm super excited at how well this is put together, and how close it is to be a part of Wagtail 🤘

It addresses all of the UX problems of nested StreamFields, which in the past many re-implementations had tried to cover over without succeeding. It also makes StreamField much faster, which was also an issue with large instances, but even not-so-large ones. And finally, it makes the code much easier to follow IMHO, all the while adding great features like duplicate and drag-n-drop.

Nonetheless there are a few issues with the implementation, but nothing that can't be addressed. Here are the high-level problems I've noticed:

  • No clear API between react-streamfield and Wagtail. That's a problem to understand how the StreamField UI gets initialised, and how its data is subsequently saved.
  • General lack of error handling. If the StreamField fails to display, it will crash and burn. If any of its content (say, a widget) throws an error, that will destroy the whole StreamField with it.
  • High risk of clashes between the CSS classes / styles used for the new implementation, and the rest of Wagtail, and third-party code. Because the class names / selectors are way too generic, and do not have namespacing, or use a naming methodology like BEM.
  • No support for translations (mostly for screen reader labels, and potential error messages).

All of these are things I would really like to see addressed before a release, as they will be sources of bugs / maintainability issues otherwise.


See all of the comments for further details / more specific issues, and the review of react-streamfield over at noripyt/react-streamfield#4 and thibaudcolas/react-streamfield#1.

Edit: I didn't do any cross-browser testing, and this is a must have before merging this


if (input.val()) {
$.ajax(window.chooserUrls.snippetChooser + modelString + '/'
+ input.val() + '/')

This comment has been minimized.

@thibaudcolas

thibaudcolas Jan 21, 2019

Member

This looks like it would need URL encoding, in case the input value isn't URL-safe.

@@ -4,17 +4,21 @@ function createSnippetChooser(id, modelString) {
var input = $('#' + id);
var editLink = chooserElement.find('.edit-link');

function snippetChosen(snippetData, initial) {
if (!initial) {

This comment has been minimized.

@thibaudcolas

thibaudcolas Jan 21, 2019

Member

Why is this necessary?

@@ -4,23 +4,26 @@ function createImageChooser(id) {
var input = $('#' + id);
var editLink = chooserElement.find('.edit-link');

function imageChosen(imageData, initial) {

This comment has been minimized.

@thibaudcolas

thibaudcolas Jan 21, 2019

Member

Same comments as snippet chooser below.

@@ -4,17 +4,21 @@ function createDocumentChooser(id) {
var input = $('#' + id);
var editLink = chooserElement.find('.edit-link');

function documentChosen(docData, initial) {

This comment has been minimized.

@thibaudcolas

thibaudcolas Jan 21, 2019

Member

Same comments as snippet chooser below.

{{ help_text }}
</div>
{% endif %}
<BlocksContainer />{# Special tag for react-streamfield #}

This comment has been minimized.

@thibaudcolas

thibaudcolas Jan 21, 2019

Member

Could you explain what this line does? It looks like invalid HTML, even if it's harmless I think it's a good goal for any code not to create HTML validation issues.

}

.content-container {
.content {

This comment has been minimized.

@thibaudcolas

thibaudcolas Jan 21, 2019

Member

This seems like a "too generic of a class name" problem.

@@ -0,0 +1,15 @@
# Sponsors

This comment has been minimized.

@thibaudcolas

thibaudcolas Jan 21, 2019

Member

It feels odd for this "Sponsors" file to only contain the sponsors of the campaign, without mentioning that those are here because of the campaign. Without any further details, I would understand this as being a list of all of the sponsors of Wagtail, of which there are many more.


editor.hallo({
toolbar: 'halloToolbarFixed',
toolbarCssClass: (closestObj.hasClass('full')) ? 'full' : (closestObj.hasClass('stream-field') && isRoot) ? 'stream-field' : '',
toolbarCssClass: (closestObj.hasClass('full')) ? 'full' : '',

This comment has been minimized.

@thibaudcolas

thibaudcolas Jan 21, 2019

Member

Should also remove the corresponding CSS,

// stream-field is added to hallotoolbar when invoked on a field within a stream-field
&.stream-field {
margin-top: -1em;
margin-left: 0;
&.affixed {
margin-top: 0;
}
}

$(window).on('resize', function() {
hot.updateSettings({
width: getWidth(),
width: '100%',

This comment has been minimized.

@thibaudcolas

thibaudcolas Jan 21, 2019

Member

Well, I'm really surprised this works. But it does. Great!

for error in non_block_errors])
return mark_safe("""
<script type="application/json" data-streamfield="%s">%s</script>
<textarea style="display: none;" name="%s">%s</textarea>

This comment has been minimized.

@thibaudcolas

thibaudcolas Jan 21, 2019

Member

Instead of rendering the data for each widget as JSON, then initialising all widgets at once when the page is done loading, did you try to initialise the widgets directly where they are defined? This would make the page appear to load faster, at the moment there is a noticeable 1-2s delay during which the page is displayed without the StreamFields.

@loicteixeira
Copy link
Contributor

loicteixeira left a comment

I had a really quick glance at the Python side of things.
It will be a huge contribution to Wagtail 👌

'label': capfirst(self.label),
'required': self.required,
'layout': self.get_layout(),
'dangerouslyRunInnerScripts': True,

This comment has been minimized.

@loicteixeira

loicteixeira Feb 1, 2019

Contributor

It's a shame that the default is to run dangerously with an eval and across all fields (the RawHtmlFieldInput field which implements this behaviour seems to be used by other components). I guess it's too late anyway.

This comment has been minimized.

@loicteixeira

loicteixeira Feb 4, 2019

Contributor

Actually we have to consider this carefully since there was a whole lot of work done in #4583 in order to eliminate all evals for CSP purposes #1288.

This comment has been minimized.

@thibaudcolas

thibaudcolas Feb 14, 2019

Member

See thibaudcolas/react-streamfield#1 (comment) for a potential solution, with an implementation that's closer to what Wagtail does at the moment (I think).

def to_json_script(data, encoder=ConfigJSONEncoder):
return json.dumps(
data, separators=(',', ':'), cls=encoder
).replace('<', '\\u003c')

This comment has been minimized.

@loicteixeira

loicteixeira Feb 1, 2019

Contributor

Wouldn't Django's built-in escapejs do the work?

if isinstance(self, (CharBlock, TextBlock, FloatBlock,
DecimalBlock, RegexBlock, URLBlock,
DateBlock, TimeBlock, DateTimeBlock,
EmailBlock, IntegerBlock)):

This comment has been minimized.

@loicteixeira

loicteixeira Feb 1, 2019

Contributor

A list like this will be the source of bug reports because it will always miss some class, especially user created ones.

return data

def get_definition(self):
definition = super(FieldBlock, self).get_definition()

This comment has been minimized.

@loicteixeira

loicteixeira Feb 1, 2019

Contributor

Since Wagtail does not support Python 2.x, there is no need to pass parameters to super (it's not the only occurrence). Old habits die hard 😄

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment