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

Improved: Add new custom block "DetailsBlock" with expanding answers and demonstrate it in a BreadPage and in a new FAQ page #425

Open
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

gzark1
Copy link
Contributor

@gzark1 gzark1 commented Jun 28, 2023

This PR is an improvement of #422. I deleted the DetailsBlock option from BaseStreamBlock, in order not to complicate things. I utilized DetailsStreamBlock in both BreadPage (and demonstrated it in "Anadama" bread) and StandardPage (demonstrated it in "FAQ"). I stored the data i created in bakerydemo/base/fixtures/bakerydemo.json, keeping only the necessary changes.

@lb-
Copy link
Member

lb- commented Jul 1, 2023

Looking much better @gzark1

Do you mind reviewing the CI error, might need to make some more tweaks to the fixtures it seems.

@cnk
Copy link
Member

cnk commented Oct 8, 2023

I have fixed the merge conflicts in the bakerydemo.json fixture. Now we are in a position to evaluate whether or not to accept this contribution.
Screenshot 2023-10-08 at 12 45 36 AM
Screenshot 2023-10-08 at 12 46 02 AM

@gzark1
Copy link
Contributor Author

gzark1 commented Oct 8, 2023

Thank you @cnk for addressing the merge conflicts. Please feel free to provide any further guidance or suggestions, and I'll promptly address them.

Copy link
Member

@thibaudcolas thibaudcolas left a 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"
Copy link
Member

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):
Copy link
Member

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?

@lb-
Copy link
Member

lb- commented Jan 31, 2024

@gzark1 are you able to rebase and review the comments above?

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.

None yet

5 participants