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

Topic page enhancements #96

Merged
merged 19 commits into from
Feb 13, 2025
Merged

Topic page enhancements #96

merged 19 commits into from
Feb 13, 2025

Conversation

zerolab
Copy link
Contributor

@zerolab zerolab commented Feb 5, 2025

What is the context of this PR?

This PR adds:

  • the ability to feature an article series and always display the latest article in the series)
    • the feature item will display the listing image, if set
  • the ability to highlight articles and/or methodologies (CMS-294)
    • the chosen items take priority and will display first in the chosen order. the rest up to 3 is backfilled from other relevant statistical articles (or methodologies, for its section)
  • an explore more StreamField block with internal and external links
  • wires everything in the template
  • misc enhancements
    • adds a label attribute to all relevant pages.
    • adds an utility function to prep a page queryset to be displayed in an onsDocumentList component

Screencast: https://www.loom.com/share/bff403cd0e8e46a3a769a1530349ffef

To-Do:

  • limit the featured series chooser to relevant series (created under the given topic)
  • limit the highlights chooser to relevant pages (created under the topic)
  • tests

How to review

  • Check the branch out locally.
  • npm run build
  • Run migrations.
  • Create or edit a topic page and sequantially fill in the various fields (features, highlighted articles, highlighted methodologies, explore more). Also create several series / articles in the series, and methodologies, under the given topic page

Follow-up Actions

  • enhance with related articles/methodologies by taxonomy, when the feature is merged.

@zerolab zerolab requested a review from a team as a code owner February 5, 2025 15:40
@zerolab zerolab marked this pull request as draft February 5, 2025 15:40
@zerolab zerolab marked this pull request as ready for review February 7, 2025 09:24
@zerolab zerolab force-pushed the topic-page-enhancements-CMS-62 branch from 3d183b6 to 5e129ab Compare February 7, 2025 09:32
AdamHawtin

This comment was marked as resolved.

Copy link
Contributor

@nehakerung nehakerung left a comment

Choose a reason for hiding this comment

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

LGTM - looks good, just have a small question.

@zerolab
Copy link
Contributor Author

zerolab commented Feb 10, 2025

@AdamHawtin can you run npm run build, that should sort out the featured block. (I missed that from the instructions, now updated in the ticket description)

@sanjeevz3009 sanjeevz3009 self-requested a review February 11, 2025 10:10
Copy link
Contributor

@AdamHawtin AdamHawtin left a comment

Choose a reason for hiding this comment

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

Looks good to me, the functionality works as expected.

I have some questions about the behaviour with highlighting/featuring draft pages. The way it's handled makes sense technically, with editors able to select to feature/highlight draft pages, but those draft pages then not appearing on the published topic page until they are published, but this isn't communicated in the editor. I wonder if it will be understood by users? Perhaps draft pages need to be highlighted in the editor with a warning that they will not show until they are published.

@sanjeevz3009
Copy link
Contributor

Looks good to me, the functionality works as expected.

I have some questions about the behaviour with highlighting/featuring draft pages. The way it's handled makes sense technically, with editors able to select to feature/highlight draft pages, but those draft pages then not appearing on the published topic page until they are published, but this isn't communicated in the editor. I wonder if it will be understood by users? Perhaps draft pages need to be highlighted in the editor with a warning that they will not show until they are published.

@AdamHawtin Echoing what Adam said. This is the same behaviour on the Generic Index page.
@kacperpONS

@zerolab
Copy link
Contributor Author

zerolab commented Feb 11, 2025

@AdamHawtin @sanjeevz3009 I hear you both. We have a backlog item that looks specifically at previews in a holistic manner (CMS-356) and one that looks at release preview in particular (CMS-350)

@kacperpONS
Copy link
Contributor

LGTM, two quick questions:

  • in your walkthrough it seems that the article titles are displayed without the article series title prefix, but when running this branch it shows both. Just wanted to clarify whether that has changed since you recorded the screencast?
  • why is the label field defined as a method? wouldn't it be sufficient if it was defined as a class variable?

Old habits die hard
@zerolab
Copy link
Contributor Author

zerolab commented Feb 11, 2025

@kacperpONS

  1. I noticed the same while doing the screencast, so followed it up in c45deb1
  2. Old habits. class var works just as well, so updated in 586f1c2. Cheers!

Copy link
Contributor

@sanjeevz3009 sanjeevz3009 left a comment

Choose a reason for hiding this comment

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

Everything looks good to me! Works as expected! One minor comment =)

@zerolab zerolab force-pushed the topic-page-enhancements-CMS-62 branch from 03cb09e to 708da0f Compare February 13, 2025 09:23
Copy link
Contributor

@sanjeevz3009 sanjeevz3009 left a comment

Choose a reason for hiding this comment

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

LGTM!

@zerolab zerolab merged commit ec5da7c into main Feb 13, 2025
9 checks passed
@zerolab zerolab deleted the topic-page-enhancements-CMS-62 branch February 13, 2025 15:41
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.

6 participants