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

Add react streamfield editor as streamfield editor #372

Closed
wants to merge 2 commits into from
Closed

Add react streamfield editor as streamfield editor #372

wants to merge 2 commits into from

Conversation

joeyjurjens
Copy link

@joeyjurjens joeyjurjens commented Oct 28, 2020

PR for #369

First of all, I got a question according the contribution guide, it states:
When making changes that are potentially destructive or backwards incompatible, increment the minor version number until coderedcms reaches a stable 1.0 release. Each production project that uses coderedcms should specify the appropriate version in its requirements.txt to prevent breakage.

I don't see a requirements.txt, so I am unsure how to update the version number, so if anyone could point this out please :)

Description of change

This PR installs the Wagtail React Streamfield editor into coderedcms, this streamfield editor makes editing a page way more easier due the ability to collapse blocks when loading the page.

As of now, it requires you to set closed = True to all blocks you want to be collapsed, while I think the other way around would be a better solution (all blocks collapsed by default). So I made a PR for that on the react streamfield editor here, which will hopefully be picked up.

If you guys still want me to add closed = True to all blocks, let me know.

Any other feedback is welcome too of course.

I've tested this by simply creating multiple pages with all sort of block types, I didn't run into a single issue.

@vsalvino
Copy link
Contributor

Awesome! The diff looks much smaller than I expected, which is great. Don't worry about incrementing the version, I will do that before making a release.

I'll test it out in the next couple days and see how it works.

Question: do you think it would be possible to support both scenarios (e.g. with react streamfield or without). E.g. if the user decides to remove it from the INSTALLED_APPS, will the system continue to work without it?

@joeyjurjens
Copy link
Author

Haha yes, I expected that too.

It's possible, if we don't remove the form_template = "**" in the AdvancedSettingsBlock.
Because that isn't used by the react streamfield editor anyways.

But then when installed, form_template does not make sense and when uninstalled the closed = True does not make sense, but this would allow both scenarios support, so I guess that's fine. What do you think?

@joeyjurjens
Copy link
Author

joeyjurjens commented Oct 28, 2020

I've re-added the form_template, this allows people to uninstall the react streamfield editor without any problems.

Something that also could be added, or maybe later once the editor is merged into wagtail, is custom block titles, to make it more clear what's inside a collapsed block. It maybe would be even more useful if users could name these themselves?

The react streamfield can already do this: https://github.com/wagtail/wagtail-react-streamfield/blob/master/wagtail_react_streamfield/blocks/struct_block.py#L25

Seems useful, just not sure if that's something for now or a later phase.

@vsalvino
Copy link
Contributor

I think we should support both for the time being, since the react streamfield is still somewhat experimental, and might cause problems with other custom wagtail setups. Although I would be inclined to make react the default.

Regarding naming the streamfield blocks... I was thinking the same thing. This would most likely be set by the user/editor; or we could possibly show a default title based on some context of the inner blocks (e.g. the title of a button, the first heading in a rich text field, etc.). This feature could be implemented for a future release though as I'm guessing it will not be quite as straight forward and might require some "bike-shedding".

@joeyjurjens
Copy link
Author

I agree, I think as long as it's clear in the next release it uses the react streamfield as default. And naming the blocks indeed seems more like a future plan/task.

I must say that the editing experience is much better now for me, though I hope wagtail merges my PR on the blocks collapsed by default setting, as this really makes it better (in my opinion), I just use my fork for now though.

@@ -165,6 +165,10 @@ class Meta:
form_template = 'wagtailadmin/block_forms/base_block_settings_struct.html'
label = _('Advanced Settings')

# Specific react streamfield settings
icon = 'cog'
closed = True
Copy link
Contributor

Choose a reason for hiding this comment

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

This does not seem to be taking effect... when I create a new block in the streamfield (in wagtail admin) the advanced settings section is open/expanded by default. Ideally this would be collapsed by default.

We will also need the ability to add a custom CSS class to this advanced settings container, so that it can be styled accordingly. That was previously achieved through the HTML template. Any ideas?

Copy link
Author

Choose a reason for hiding this comment

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

Hmm, I see. The closed option in the meta class is for when you load a page. So if you save your page and then check it should be closed. I understand that it currently is collapsed when adding the block & I'll check if it's possible to have the same effect and let you know.

I assume we can still use wagtail_hooks to inject custom CSS into the streamfield editor, but I'll have to test this out & let you know.

Copy link
Author

Choose a reason for hiding this comment

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

If we want to achieve the same behaviour and looks, we have to modify the react-streamfield code. I'm not that familiar with JS/React as in overriding code, so I'll look into that tomorrow and see if I can do this.

Copy link
Contributor

Choose a reason for hiding this comment

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

That makes sense. Thanks for doing all this leg work Joey - it is a big help!

Copy link
Author

Choose a reason for hiding this comment

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

I did some research, and there's no easy way of extending the editor unfortunately.

I could fork the editor, but I'm not sure how the react editor will be implemented in wagtail itself; will it stay a separate repo or will it completely be merged into wagtail. If it's option two, I don't know if it's wise for you guys to use a fork in coderedcms, as when wagtail releases it the fork makes no sense.

I've currently forked it with my changes which makes it work like it works now with the advanced settings, so it is possible with a fork.

Do you maybe happen to know how it's going to be merged in wagtail? And what do you think, is it wise for you guys to use a fork?

Copy link
Contributor

@vsalvino vsalvino Nov 2, 2020

Choose a reason for hiding this comment

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

I would like to avoid using a fork if possible, only because that presents a lot of ongoing maintenance. I would encourage you to try and submit a patch to wagtail-react-streamfield if possible. If so, could you link that PR here as well? I would be happy to help with that patch if you'd like another set of eyes.

I don't know when it will be merged into wagtail. Last I heard from Matt Westcott and team in July was "it's on our radar but no ETA".

An alternate approach for us might be to make the necessary changes to support the react streamfield, without adding it to requirements/installed_apps. And let folks start using it on their own, "use at your own risk" situation to test it out. We could then point them to your fork as experimental.

Copy link
Author

@joeyjurjens joeyjurjens Nov 2, 2020

Choose a reason for hiding this comment

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

I agree that using forks is not the ideal. I will submit my PR to wagtail, I've submitted one PR's on react-streamfield and one on wagtail-react-streamfield, but as of now I haven't got any replies on it yet so doubt it will be any different.

Those are the only changes that are required: https://github.com/joeyjurjens/react-streamfield/commit/d4685c06aea3e77c171283d776e2cff10a78bad9

I'll make a PR once I get home, but unsure if they'll do anything with it 😅 but we'll see hehe.

Copy link
Author

@joeyjurjens joeyjurjens Nov 2, 2020

Choose a reason for hiding this comment

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

I made a PR, now it's waiting haha; wagtail-deprecated/react-streamfield#13

@vsalvino vsalvino added the PR: Blocked Block by another issue that must be resolved first. label Jan 11, 2021
@vsalvino
Copy link
Contributor

vsalvino commented Aug 2, 2021

@joeyjurjens is this still relevant in Wagtail 2.13? I believe they switched to a react-based streamfield. Not sure if the built-in streamfield is different than the react-streamfield package?

@joeyjurjens
Copy link
Author

@joeyjurjens is this still relevant in Wagtail 2.13? I believe they switched to a react-based streamfield. Not sure if the built-in streamfield is different than the react-streamfield package?

I have no idea to be honest. I'll check it out tonight and let you know. Though, this PR depends on other PR's which are unlikely to be merged as I've had no response at all. So maybe this PR should be closed already.

@joeyjurjens
Copy link
Author

@vsalvino Wagtail v2.13 indeed has the react-streamfield, so I'm closing this PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR: Blocked Block by another issue that must be resolved first.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants