-
Notifications
You must be signed in to change notification settings - Fork 39
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
Change help text in stream field to RichText #4375
Conversation
@wes-otf This PR removes the help link field and migrate any existing links as markdown links and the end of any help text. The help text field already accepts and renders any markdown you add to it. What's missing yet is a markdown editor for the help field, if that is needed. |
For our part, a markdown editor is important to our clients. |
Tested a number of ways to get a Markdown editor on the help text field but found no clean way of doing it. Current version converts the field in to a Wagtail RichTextBlock instead, just like @frankduncan PR. |
The best thing we've found in a different project was importing the js-only library wysimark and writing javascript to populate and pull the information out. I'm not sure how that would interact with the wagtail admin stuff. I know wagtail has some javascript events (we used it in our email configuration code) that you can hook into to do things. |
Normally I default to markdown but in this case I think rich text is the better option. All other fields that have formatting are already using the wagtail rich text editor. The consistency is appreciated by staff I think. The editor is quite nice as well. This PR is now very much a copy of #3293 with the added migrations of the help link field. |
I had no idea the help text fields would render markdown as is, basic formatting in help text has been a request from staff for a little while so that info will fix some issues in in the interim. I see no issues with converting this to RichText though. It looks like the help text fields are stored in raw markdown format so we'll also probably need a migration that converts existing fields into HTML |
@@ -61,15 +61,6 @@ | |||
<div class="form__help prose prose-sm">{{ field.help_text|markdown|nh3 }}</div> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this could also probably be tossed right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch, yes the markdown filter can go and likely the nh3 as well since this is not user generated.
Or we could consider adding nh3 to all outputs, as an extra safety measure.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@wes-otf If OTF is not using Markdown in the help text as of now I suggest we get this deployed soon and avoid more migrations.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OTF isn't using markdown but even plaintext will still need to be wrapped in a <p>
tag for rich text (if I'm understanding how richtext is stored) so a migration might be necessary. not sure if any adopters have used it either
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
also makes sense to use nh3, maybe we should also do that for paragraph blocks too just for uniformity
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The nh3 filter gives an error used alone here, unsure why but it is not really needed anyway so removed it.
The migration is working as it should in my testing. Old as well as new text are rendered correctly with p-tags and all.
779815d
to
88ccdc8
Compare
@wes-otf Can you give this a test locally before we deploy it to the test server? Some complex migrations that would mess up the test db if there are bugs. You can revert the test tb but it is a bit of a hassel. |
yes! I'll try it out today |
I think it's ready for test! tested it with existing help text strings (non-markdown) and things migrated totally fine and the rich text displayed as expected |
@frankduncan just wanted to confirm none of your adopters use markdown in the help text field currently? |
@wes-otf Correct, none of them use markdown. |
Fixes #3293
Test Steps