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

Wagtail 2.13 compatibility #81

Merged
merged 2 commits into from
Jun 15, 2021
Merged

Conversation

zerolab
Copy link
Member

@zerolab zerolab commented Jun 13, 2021

Fixes #79

Adds a new MarkdownTextarea adapter that takes care of the magic. Wagtail 2.13 now uses the telepath library to map data between the widget and the StreamField representation. More at
https://docs.wagtail.io/en/latest/reference/streamfield/widget_api.html


// define public API functions for the widget:
// https://docs.wagtail.io/en/latest/reference/streamfield/widget_api.html
return {

Choose a reason for hiding this comment

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

According to https://docs.wagtail.io/en/stable/reference/streamfield/widget_api.html, the object returned should have an idForLabel attribute.

Copy link
Member Author

Choose a reason for hiding this comment

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

heh, good spot. added.

it works without it. I read that as not providing it or setting it as null to be the same thing

Choose a reason for hiding this comment

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

Compared to https://github.com/wagtail/wagtail/blob/cdeed1954e3498ea2acab1ca43b2bdffb23af56e/client/src/entrypoints/admin/telepath/widgets.js#L9, it looks like it sets its own idForLabel based on the id provided. Clearly null idForLabel is valid, but based on https://github.com/wagtail/wagtail/blob/cee245e7015b8462f2aa921e42803cd7d0962b8b/client/src/components/StreamField/blocks/StructBlock.js#L70, I suspect it might be worse for accessibility if the label is missing that for. Think it might be worth setting this to the id.

Copy link
Member Author

Choose a reason for hiding this comment

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

The markdown block does not provide a label as the main input is a hidden text area and the visual bit is a rendered editor. The block, in many ways is similart to the rich text block ones. The Draftail widget does not provide an idForLabel either - https://github.com/wagtail/wagtail/blob/cdeed1954e3498ea2acab1ca43b2bdffb23af56e/client/src/entrypoints/admin/telepath/widgets.js#L150-L183.

one for @thibaudcolas to hopefully guide us on. for the time being will stick to the null value and update it later to follow DraftailRichTextArea

Choose a reason for hiding this comment

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

Ah I see - my mistake, sorry about that!

@zerolab zerolab requested a review from jacobtoppm June 14, 2021 13:11
Copy link

@jacobtoppm jacobtoppm left a comment

Choose a reason for hiding this comment

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

Not a telepath expert, but generally looks good to me, just the idForLabel to potentially address


// define public API functions for the widget:
// https://docs.wagtail.io/en/latest/reference/streamfield/widget_api.html
return {

Choose a reason for hiding this comment

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

Compared to https://github.com/wagtail/wagtail/blob/cdeed1954e3498ea2acab1ca43b2bdffb23af56e/client/src/entrypoints/admin/telepath/widgets.js#L9, it looks like it sets its own idForLabel based on the id provided. Clearly null idForLabel is valid, but based on https://github.com/wagtail/wagtail/blob/cee245e7015b8462f2aa921e42803cd7d0962b8b/client/src/components/StreamField/blocks/StructBlock.js#L70, I suspect it might be worse for accessibility if the label is missing that for. Think it might be worth setting this to the id.

@zerolab zerolab merged commit 21cfae2 into main Jun 15, 2021
@zerolab zerolab deleted the chore/wagtail-2.13-compatibility branch June 15, 2021 18:24
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

Successfully merging this pull request may close these issues.

MarkdownBlock Empty after saving in Wagtail 2.13
2 participants