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

New announcement system for TheyWorkForYou #1711

Merged
merged 4 commits into from
Jun 6, 2023

Conversation

ajparsons
Copy link
Contributor

This PR adds a more general announcement feature to TheyWorkForYou.

  • This extends the current Banner approach to allow multiple banners to rotate at once.
  • New 'announcements' can be made in three locations: the homepage, MP sidebar, and a donation box in the MP sidebar.

The three locations style content in slightly different ways:

  • 'homepage' - uses all the content, but is quite wide rather than in a sidebar.
  • 'sidebar' - uses image, title and button options, but does not display the additional 'content' option.
  • 'donation' - uses title, content and button options (no image).

The backend has been adapted to store JSON that stores details of multiple options. Format is shown here. Invalid Json throws an error (but is not checked against a specific schema):

Dev helpers:

For consistency with the script-to-rule-them-all approach elsewhere the 'script' directory as opposed to 'script', stores several useful management commands for use in dev. Currently this contains watch-css and `create-superuser'.

Other styling changes:

  • The announcement section on homepage has demoted the 'featured' debate to below the search bar.
  • A new green 'Browse content' helps draw attention to the sidebar on MPs profiles.

image

image

image

@ajparsons ajparsons mentioned this pull request Apr 24, 2023
@ajparsons ajparsons requested a review from dracos April 24, 2023 15:33
@ajparsons
Copy link
Contributor Author

ajparsons commented Apr 25, 2023

Things not covered in this PR:

  • This doesn't cover - but probably should - adding announcements to alerts. Will do this as a future PR because testing it is slightly different.

Copy link
Member

@dracos dracos left a comment

Choose a reason for hiding this comment

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

Nothing major, but a few bugs and things to fix

classes/Model/AnnoucementManagement.php Outdated Show resolved Hide resolved
classes/Model/AnnoucementManagement.php Outdated Show resolved Hide resolved
classes/Model/AnnoucementManagement.php Outdated Show resolved Hide resolved
www/docs/admin/banner.php Outdated Show resolved Hide resolved
www/docs/admin/banner.php Outdated Show resolved Hide resolved
<?php } else { ?>

<?php if ( count($featured) > 0 ) {
include 'featured.php';
Copy link
Member

Choose a reason for hiding this comment

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

This means the featured.php will be shown twice on the page if there's no homepage announcement.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I couldn't see a good way around this because some content we want in this box above the search (the alerts link) on the right all the time. My assumption (which hopefully won't be wrong) is we'll always have something there, and if not, it won't be immediately apparent to most people landing it's duplicated above and below the search.

Copy link
Member

Choose a reason for hiding this comment

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

We could set a global flag so if it's being included the first time and the second time checks if the flag has been set and doesn't show it again, but as you say, no biggie.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've done this just so it isn't a surprise later (or when we first turn it on). Looks like it works fine.

classes/Model/AnnoucementManagement.php Outdated Show resolved Hide resolved
@ajparsons
Copy link
Contributor Author

Should have fixed all of the big things and it looks like it works okay.

If seems alright, I'll fix up the history to something sensible.

@ajparsons ajparsons requested a review from dracos June 5, 2023 12:56
Copy link
Member

@dracos dracos left a comment

Choose a reason for hiding this comment

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

Just the one bug I think, and one query, is all.

@ajparsons
Copy link
Contributor Author

OK! Should be almost there.

@ajparsons ajparsons requested a review from dracos June 6, 2023 10:33
Copy link
Member

@dracos dracos left a comment

Choose a reason for hiding this comment

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

There's one issue as to whether the new if is around a bit too much, but not very important :)

@@ -93,6 +94,7 @@
</div>
</div>

<?php if ( $featured_debate_shown == false ) { ?>
Copy link
Member

Choose a reason for hiding this comment

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

Should this if go round just the lines 101-105, otherwise the topics aren't shown if featured has been shown above?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep, can do that.

ajparsons and others added 4 commits June 6, 2023 11:34
- Stores json config in 'banner' and 'announcements' editoral option.
- Checks valid json (not valid schema) before save.
 - use 'script' directory to mirror script-to-rule-them-all elsewhere
 - shortcut for css updating
 - create-superuser script for use in development envs
 - shortcut for changing cookie-domain in codespaces
- Add new Announcement box above search
- Falls back to featured/random debate
- Featured/random debate seperately shown beneath search

Co-authored-by: Lucas <[email protected]>
- Adds announcement location to MP sidebar
- Adds 'donation' location to MP sidebar
- Adds 'Browse content' green box to draw attention to sidebar.

Co-authored-by: Lucas <[email protected]>
@ajparsons
Copy link
Contributor Author

Okay! History tidied up (and spelling fixed). Will merge in this afternoon!

@ajparsons ajparsons merged commit 95eeb3d into master Jun 6, 2023
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