-
Notifications
You must be signed in to change notification settings - Fork 544
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
Improved: Add new custom block "DetailsBlock" with expanding answers and demonstrate it in a BreadPage and in a new FAQ page #425
base: main
Are you sure you want to change the base?
Conversation
…emo.json, keeping only necessary changes
Looking much better @gzark1 Do you mind reviewing the CI error, might need to make some more tweaks to the fixtures it seems. |
Thank you @cnk for addressing the merge conflicts. Please feel free to provide any further guidance or suggestions, and I'll promptly address them. |
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.
I think this is great – but I would say this ought to be in the body
field, so we can demonstrate Wagtail’s support for flexible page content.
summary = CharBlock(required=True) | ||
content = RichTextBlock(required=True) | ||
open = BooleanBlock( | ||
required=False, default=True, label="Open", help_text="Open by default" |
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.
I’m not too clear why we are using a <details>
element in HTML if it’s to have those blocks opened by default. Could we default to "False" instead?
I’d also suggest removing the label if it just repeats the field identifier, and writing a more descriptive help text or removing it (stating the default value isn’t too helpful).
|
||
|
||
# StreamBlocks for Details | ||
class DetailsStreamBlock(StreamBlock): |
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.
I’m really surprised we’re not adding this to BaseStreamBlock
– this feels like a clear missed opportunity to demonstrate Wagtail’s flexibility, by allowing series of "details" content to be placed at arbitrary points?
@gzark1 are you able to rebase and review the comments above? |
This PR is an improvement of #422. I deleted the DetailsBlock option from
BaseStreamBlock
, in order not to complicate things. I utilizedDetailsStreamBlock
in bothBreadPage
(and demonstrated it in "Anadama" bread) andStandardPage
(demonstrated it in "FAQ"). I stored the data i created inbakerydemo/base/fixtures/bakerydemo.json
, keeping only the necessary changes.