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

Fix event image/venue map logic #8603

Merged
merged 6 commits into from
Oct 15, 2024
Merged

Fix event image/venue map logic #8603

merged 6 commits into from
Oct 15, 2024

Conversation

csebianlander
Copy link
Contributor

@csebianlander csebianlander commented Oct 9, 2024

This PR removes several fields and moves another in order to simplify and clarify the logic around selecting a venue image and/or generated map for an Event page. It also fixes an issue where a large CFPB logo image was shown for all events once concluded, regardless of selections made in any of these fields.


Removals

  • Unused but visible archive_image field
  • Unnecessary post_event_image field and related unit test

Changes

  • Moved venue_image tab to the General Content tab and relabeled

How to test this PR

  1. localhost
  2. Create or edit an Event page
  3. Use the moved "Image" fields in the General Content tab. Note that maps don't appear to render on localhost, but you will see a change in the spacing of the image area that indicates an attempt to generate

Notes and todos

  • This is not intended to be a perfect or complete solution to Event page woes but instead an iteration that fixes a longstanding problem and removes some cruft. There are many other changes we could integrate into a PR like this, but I think we should wait for any larger-scale changes to code until we have a more focused effort to review it.

Checklist

  • PR has an informative and human-readable title
    • PR titles are used to generate the change log in releases; good ones make that easier to scan.
    • Consider prefixing, e.g., "Mega Menu: fix layout bug", or "Docs: Update Docker installation instructions".
  • Changes are limited to a single goal (no scope creep)
  • Code follows the standards laid out in the CFPB development guidelines
  • Future todos are captured in comments and/or tickets
  • Project documentation has been updated, potentially one or more of:
    • This repo’s docs (edit the files in the /docs folder) – for basic, close-to-the-code docs on working with this repo
    • CFGOV/platform wiki on GHE – for internal CFPB developer guidance
    • CFPB/hubcap wiki on GHE – for internal CFPB design and content guidance

Copy link
Member

@willbarton willbarton left a comment

Choose a reason for hiding this comment

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

I am not entirely familiar with the event logic, but I think the main thing is, if we're not using the post_event_image fields anymore, I think we should remove them.

cfgov/v1/models/learn_page.py Show resolved Hide resolved
cfgov/v1/jinja2/v1/events/_macros.html Show resolved Hide resolved
@csebianlander csebianlander added this pull request to the merge queue Oct 15, 2024
Merged via the queue into main with commit 596ba96 Oct 15, 2024
12 checks passed
@csebianlander csebianlander deleted the event-venue-image branch October 15, 2024 15:00
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