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

Add bgcolor and textcolor fields to DonateBanner model #12986

Merged

Conversation

ramram-mf
Copy link
Collaborator

@ramram-mf ramram-mf commented Oct 9, 2024

Description

This PR updates the DonateBanner model with the addition of two new fields background_color and text_color, allowing the user to customize the donate banner present on all site pages. It also renames the previously incorrectly named background_image to foreground_image and adds a background_image field to store the newly implemented background image feature for the banner.

To test

  1. Open the review app Wagtail auth page, authenticate with admin2:admin2 credentials.
  2. Navigate to the Settings > Donate banner page, click on the Choose in order to pick a banner to show.
image
  1. Select a banner from the list by clicking on its name. Save by clicking on the Save button on the bottom of the page.
image image
  1. Edit the selected banner by clicking on the three dots next to its name and then selecting the Edit menu:
image
  1. Note the new model fields Background image, Background color and Text color. Customize the background color of the banner. Save and browse the preview app homepage.
    image

  2. Confirm that the background color has changed in the homepage.

image
  1. Edit again the donate banner and upload a background image. Save and confirm changes reflect on the homepage.
image

Link to sample test page: https://foundation-s-tp1-1234-i-bhpnku.herokuapp.com/en/
Related PRs/issues: #12881

┆Issue is synchronized with this Jira Story

@ramram-mf ramram-mf temporarily deployed to foundation-s-tp1-1234-i-hlbzy3 October 29, 2024 15:33 Inactive
@ramram-mf ramram-mf marked this pull request as ready for review October 29, 2024 18:05
@ramram-mf ramram-mf temporarily deployed to foundation-s-tp1-1234-i-hlbzy3 October 29, 2024 18:14 Inactive
Copy link
Collaborator

@mmmavis mmmavis left a comment

Choose a reason for hiding this comment

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

Hey @ramram-mf , thanks for adding these customization options for the donate banner. I have left a few comments that I think would help with UX and code maintainability.

network-api/networkapi/donate_banner/models.py Outdated Show resolved Hide resolved
network-api/networkapi/donate_banner/models.py Outdated Show resolved Hide resolved
network-api/networkapi/donate_banner/models.py Outdated Show resolved Hide resolved
@ramram-mf ramram-mf temporarily deployed to foundation-s-tp1-1234-i-hlbzy3 October 30, 2024 03:46 Inactive
@ramram-mf ramram-mf temporarily deployed to foundation-s-tp1-1234-i-hlbzy3 October 30, 2024 04:03 Inactive
@ramram-mf ramram-mf temporarily deployed to foundation-s-tp1-1234-i-hlbzy3 October 30, 2024 04:11 Inactive
@ramram-mf ramram-mf temporarily deployed to foundation-s-tp1-1234-i-hlbzy3 October 30, 2024 04:21 Inactive
@ramram-mf ramram-mf self-assigned this Oct 30, 2024
@ramram-mf ramram-mf temporarily deployed to foundation-s-tp1-1234-i-bhpnku October 30, 2024 04:40 Inactive
@ramram-mf ramram-mf temporarily deployed to foundation-s-tp1-1234-i-bhpnku October 30, 2024 17:39 Inactive
@beccaklam
Copy link

Hi @ramram-mf! Thank you for your work on this, it's looking good 👍 Just adding the notes we discussed:

  1. I think we should limit the colours to the following for now: black, white, Red 40 #FF4F5E and Blue 40 #595CF3
  2. The background colour option should be disabled when a user has chosen to upload a background image so that it's clear the background image will load instead of the background colour. If the user clears the background image choice, the background colour will be enabled again.

@ramram-mf ramram-mf temporarily deployed to foundation-s-tp1-1234-i-bhpnku October 30, 2024 19:10 Inactive
@ramram-mf ramram-mf temporarily deployed to foundation-s-tp1-1234-i-bhpnku October 30, 2024 20:44 Inactive
@ramram-mf ramram-mf temporarily deployed to foundation-s-tp1-1234-i-bhpnku October 30, 2024 21:34 Inactive
@ramram-mf ramram-mf temporarily deployed to foundation-s-tp1-1234-i-bhpnku October 30, 2024 21:34 Inactive
…n-fo' of github-mf:MozillaFoundation/foundation.mozilla.org into TP1-1234-investigate-background-font-color-customization-fo
@ramram-mf ramram-mf temporarily deployed to foundation-s-tp1-1234-i-bhpnku October 30, 2024 21:38 Inactive
Copy link

@beccaklam beccaklam left a comment

Choose a reason for hiding this comment

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

Thank you for making those changes @ramram-mf! This looks good to me.

Copy link
Collaborator

@danielfmiranda danielfmiranda left a comment

Choose a reason for hiding this comment

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

Hi @ramram-mf!

Thanks very much for taking this work on. After testing this out on the review app, I can confirm that everything is working as expected. I appreciate you fixing the indexing issue when it came to repurposing the background_image field name, and transferring over existing data. Great work!

I liked your approach of disabling the background_color field when a background image is selected.

However, I have a slight concern that users might find it a bit confusing without immediate context, as there isn’t any help text to clarify why background_color is disabled when both fields are set, until they hit “Save.”

To simplify things and add clarity for users, I’d suggest leveraging Wagtail’s MultiFieldPanel and HelpPanel. This would allow us to provide clear instructions right within the CMS interface. Here’s an example:

MultiFieldPanel(
    [
        HelpPanel(content="Select either an image or a color to serve as the background for the banner."),
        FieldPanel("background_image"),
        FieldPanel("background_color"),
    ],
    heading="Background",
)

With this approach, the CMS will display clear instructions before users make their selection:

Screenshot 2024-10-31 at 00-03-49 Editing donate banner - B2 - Wagtail

We can then handle validation in the models clean() method.

Here's what that might look like:

    def clean(self):
        super().clean()

        both_selected_error = "Please select either a background image or a background color for the banner."
        none_selected_error = "Please select a background image or a background color for the banner."

        # Validate that either background_image or background_color is set, not both.
        if self.background_image and self.background_color:
            raise ValidationError(
                {
                    "background_image": ValidationError(both_selected_error),
                    "background_color": ValidationError(both_selected_error),
                }
            )
        if not self.background_image and not self.background_color:
            raise ValidationError(
                {
                    "background_image": ValidationError(none_selected_error),
                    "background_color": ValidationError(none_selected_error),
                }
            )

This way, if both or none of the fields are set, they’ll see a styled validation error reminding them to choose just one:

Screenshot 2024-10-31 at 00-08-41 Editing donate banner - B2 - Wagtail

This update could help streamline the user experience and keep the codebase leaner by removing the two new files needed for the custom disabling logic. Let me know what you think!

Thanks again for your hard work!

@ramram-mf ramram-mf temporarily deployed to foundation-s-tp1-1234-i-bhpnku October 31, 2024 16:59 Inactive
…n-fo' of github-mf:MozillaFoundation/foundation.mozilla.org into TP1-1234-investigate-background-font-color-customization-fo
@ramram-mf ramram-mf temporarily deployed to foundation-s-tp1-1234-i-bhpnku October 31, 2024 17:16 Inactive
@ramram-mf ramram-mf temporarily deployed to foundation-s-tp1-1234-i-bhpnku October 31, 2024 18:29 Inactive
Copy link
Collaborator

@danielfmiranda danielfmiranda left a comment

Choose a reason for hiding this comment

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

Thanks @ramram-mf! Great work 🙌 LGTM

Copy link
Collaborator

@mmmavis mmmavis left a comment

Choose a reason for hiding this comment

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

Nice work @ramram-mf ! Also thank you @danielfmiranda for your great suggestion.

@ramram-mf ramram-mf temporarily deployed to foundation-s-tp1-1234-i-bhpnku October 31, 2024 20:28 Inactive
@ramram-mf ramram-mf merged commit 3df8816 into main Oct 31, 2024
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants