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

Remove Title from PanelBlock #90

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

Conversation

sanjeevz3009
Copy link
Contributor

@sanjeevz3009 sanjeevz3009 commented Jan 29, 2025

What is the context of this PR?

The latest ONS Design System latest release meant that the title option for warning, announcement and bare panels has been removed. The PR for this is in the ONS DS.

Currently, in our Wagtail implementation, we still have the warning and announcement panels with the title which is redundant now.

  • This PR removes the title field in Wagtail for the choices: Warning and Announcement.
  • Keeps the title field in Wagtail for the choices: Information, Error and Success.

How to review

  • Validate that on the information page, you can see the relevant panels when adding content such as "Info/Error/Success Panel" and "Warning/Announcement Panel".
  • The "Warning/Announcement Panel" shouldn't contain a title.
  • Make sure this behaviour is also reflected in the article and methodology page.

Follow-up Actions

List any follow-up actions (if applicable), like needed documentation updates or additional testing.

label = _("Warning/Announcement Panel")


class InfoErrorSuccessPanelBlock(blocks.StructBlock):
Copy link
Contributor

Choose a reason for hiding this comment

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

Note that following further info based on user research, this is likely not needed either. So we would have an information/warning only. Can you confirm with Gill/Julia and Paula.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will do!

Copy link
Contributor

Choose a reason for hiding this comment

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

also, let's split them into separate blocks:

WarningPanelBlock
InformationPanelBlock
AnnouncementPanelBlock

and all defined with a group=_("Panels") in their corresponding StreamField blocks.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done!

@helenb
Copy link
Contributor

helenb commented Jan 30, 2025

Thanks @sanjeevz3009 I have functionally tested and working as described. I agree with @zerolab (against my own earlier suggestions) that it makes sense to have separate blocks for each panel type now, especially if there are only 3.

@sanjeevz3009 sanjeevz3009 marked this pull request as ready for review January 31, 2025 01:57
@sanjeevz3009 sanjeevz3009 requested a review from a team as a code owner January 31, 2025 01:57
@sanjeevz3009
Copy link
Contributor Author

Thanks @sanjeevz3009 I have functionally tested and working as described. I agree with @zerolab (against my own earlier suggestions) that it makes sense to have separate blocks for each panel type now, especially if there are only 3.

Code updated. Let me know what you think now @helenb

Copy link
Contributor

@helenb helenb left a comment

Choose a reason for hiding this comment

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

Thanks @sanjeevz3009 I have tested again and all functions as expected. Added a couple of nitpicks (and will wait to approve till we know about the announcement panel)


def get_context(self, value: dict, parent_context: dict | None = None) -> dict:
context: dict = super().get_context(value, parent_context)
context["variant"] = "info"
Copy link
Contributor

Choose a reason for hiding this comment

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

Nitpick: as this has it's own template, we don't need to pass the variant through here - it will always be 'info'

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Very true. I guess my brain was on some weird autopilot/ copy-paste mentality from one class to another haha. Or I think I was doing something then I forgot to remove it, I can't remember. Removed!


def get_context(self, value: dict, parent_context: dict | None = None) -> dict:
context: dict = super().get_context(value, parent_context)
context["variant"] = "warn"
Copy link
Contributor

Choose a reason for hiding this comment

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

If we remove the announcement panel, we also won't need to pass the variant to the context here - and the template could also be renamed for clarity.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will update this accordingly once I receive confirmation on if we still need the announcement panel.

@sanjeevz3009
Copy link
Contributor Author

sanjeevz3009 commented Jan 31, 2025

Thanks @sanjeevz3009 I have tested again and all functions as expected. Added a couple of nitpicks (and will wait to approve till we know about the announcement panel)

Thanks @helenb Will address them. Contacted the relevant people and chased up the confirmation for the announcement panel. Hopefully we get a response soon.

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.

3 participants