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

[#8826] cms/how-to-take-part: add how to take part block #6024

Open
wants to merge 1 commit into
base: dev
Choose a base branch
from

Conversation

vellip
Copy link
Collaborator

@vellip vellip commented Jan 31, 2025

Describe your changes
Add how to take part block to cms and remove unused ImageCallToActionBlock

Tasks

  • PR name contains story or task reference
  • Steps to recreate and test the changes
  • Documentation (docs and inline)
  • Tests (including n+1 and django_assert_num_queries where applicable)
  • Changelog

@vellip vellip requested a review from hom3mad3 January 31, 2025 08:53
Copy link
Contributor

@hom3mad3 hom3mad3 left a comment

Choose a reason for hiding this comment

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

In this class, HowToTakePartBlock, the name is currently focused on the use case rather than the elements. To make the naming more aligned with the structural elements and provide better flexibility for future use cases, we should consider a name that reflects the block's structural components.

@vellip vellip force-pushed the pv-2025-01-how-to-take-part-block branch from 05ccb34 to cb40a0e Compare February 4, 2025 17:22
@vellip vellip force-pushed the pv-2025-01-how-to-take-part-block branch from cb40a0e to 42bce21 Compare February 4, 2025 17:23
@vellip vellip requested a review from hom3mad3 February 4, 2025 17:23
icon = "image"


class ColoredPanelsBlock(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.

Thanks for the update—this is definitely better! However, I think we should consider adding icon list somewhere, since we're not passing anything else to this block. That might help clearly represent its purpose in the CMS. Sorry if my previous review was unclear or misleading 😕

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.

2 participants