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

Dont show banner on single calendar event pages #789

Conversation

StevenSawtelle
Copy link
Contributor

@StevenSawtelle StevenSawtelle commented Jul 25, 2024

Addresses issue #435

Adds a class to body that doesn't show the banner on any calendar/x pages, but still shows it on /calendar. Other options I considered (well, Simon considered let's be real):

  • Remove the banner entirely from the page in the <script>
  • Add a class to the pp-banner itself that gives it a display: none
  • Try and modify the hugo template to not show the banner on single event pages, but this seems like it would require a huge refactor to hugo and I got scared

Calendar page:
Screenshot 2024-07-24 at 8 50 59 PM

Event page:
Screenshot 2024-07-24 at 8 51 18 PM

@ionous
Copy link
Contributor

ionous commented Jul 30, 2024

re:

"Try and modify the hugo template to not show the banner on single event pages, but this seems like it would require a huge refactor to hugo and I got scared"

i logged an issue for making separate hugo pages for things.
i think that'll be a good long term direction. ( and doesn't have to block this issue or this pr )
#790

@ionous
Copy link
Contributor

ionous commented Jul 30, 2024

looking this over again, i think this idea that @StevenSawtelle and i worked out is a decent one.
i think -- because the class addition isn't specific to the pp-promo-banner it might be better to live in events.html -- maybe here:

where there is a (jquery) on ready.
we could probably take this, and then move it later if we wanted.

@carrythebanner carrythebanner merged commit 5d7dfac into shift-org:main Aug 13, 2024
4 checks passed
@carrythebanner
Copy link
Collaborator

Thanks @StevenSawtelle!

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