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

Set maximum number of stories in a flexible general container #27708

Merged
merged 19 commits into from
Jan 22, 2025

Conversation

Georges-GNM
Copy link
Contributor

@Georges-GNM Georges-GNM commented Jan 9, 2025

What is the value of this and can you measure success?

Closes this fairground ticket, making use of the value getting populated following this fronts tool PR (though that value is effectively optional, so this PR can be merged irrespective of the fronts tool one).

The maxItemsToDisplay value, which is surfaced in the displayHints field of the CollectionConfig, is now used to set the maximum number of stories appearing in a flexible general collection when it gets pressed.

The PR includes a couple of incidental changes, including deleting the FlexibleContainers object which is unused (and most likely was added when the flexible containers were implemented in a symmetrical fashion as the dynamic containers), and some formatting after running scalafmt.

Screenshots

A flexible general container showing only the desired 5 stories from the fronts tool - whereas previously the container would include the pieces below the dividing line in the tool:

image

Testing

Deploy this to code, and in the fronts tool (running the branch from the above PR), set a specific number of maximum stories for a flexible general container in the config tool. The front should get re-pressed, so checking on the code front, does the desired number of stories get shown?

You can also check the pressed json in s3.

If this DCR branch is running on code (or maybe already merged), you can also confirm larger collections (9+ stories) are displaying the expected number of stories.

Checklist

@Georges-GNM Georges-GNM force-pushed the gl/flex-gen-visible-stories branch 2 times, most recently from 73f915f to 1759337 Compare January 14, 2025 14:22
@Georges-GNM Georges-GNM changed the title Passing max items to flex gen slice definition Set maximum number of stories in a flexible general container Jan 14, 2025
@Georges-GNM Georges-GNM self-assigned this Jan 14, 2025
@Georges-GNM Georges-GNM requested a review from abeddow91 January 14, 2025 16:23
@Georges-GNM Georges-GNM marked this pull request as ready for review January 14, 2025 16:23
@Georges-GNM Georges-GNM requested a review from a team as a code owner January 14, 2025 16:23
@abeddow91 abeddow91 requested review from Fweddi and domlander January 16, 2025 09:49
@Georges-GNM Georges-GNM force-pushed the gl/flex-gen-visible-stories branch from 1759337 to fe09280 Compare January 21, 2025 10:53
@Georges-GNM Georges-GNM force-pushed the gl/flex-gen-visible-stories branch from fe09280 to 0bdeb57 Compare January 21, 2025 10:54
@Fweddi Fweddi self-assigned this Jan 21, 2025
Copy link

@Fweddi Fweddi left a comment

Choose a reason for hiding this comment

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

LGTM!

Two small points:

  • It's possible to botch the container by including a negative number.
  • It's unclear that there is a limit of 20 within the tooling itself.

@Georges-GNM
Copy link
Contributor Author

LGTM!

Two small points:

  • It's possible to botch the container by including a negative number.
  • It's unclear that there is a limit of 20 within the tooling itself.

Thanks, good points! That feels like config tool issues, so will have a look at those there.

@Georges-GNM Georges-GNM merged commit 025bfef into main Jan 22, 2025
6 checks passed
@Georges-GNM Georges-GNM deleted the gl/flex-gen-visible-stories branch January 22, 2025 09:48
@prout-bot
Copy link
Collaborator

Seen on ADMIN-PROD (merged by @Georges-GNM 11 minutes and 24 seconds ago)

@prout-bot
Copy link
Collaborator

Seen on FRONTS-PROD (merged by @Georges-GNM 11 minutes and 39 seconds ago)

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.

3 participants