Add editable maxItems field in Config tool #1763

Fweddi · 2025-02-21T14:15:20Z

What's changed?

Add an editable maxItems field per 'group' (e.g. splash, standard) in a given container.

This allows the new flexible/general container to have different card groups with variable card limits.

Intended to replace the use of the maxItemsToDisplay field. Note this PR does not enforce any card limits in the Fronts Tool, this will be carried out in a future PR.

Note you will need to 'reset' the container to see the groupsConfig defaults pull through. So if you are looking at a flexible/general container in the config, you will need to change the container type to something else and then back to flexible/general.

Before	After

Fweddi · 2025-02-21T14:16:03Z

build.sbt

@@ -83,7 +83,7 @@ libraryDependencies ++= Seq(
  "com.gu" %% "content-api-client-aws" % "0.7.6",
  "com.gu" %% "content-api-client-default" % capiClientVersion,
  "com.gu" %% "editorial-permissions-client" % "3.0.0",
-  "com.gu" %% "fapi-client-play30" % "15.0.0",
+  "com.gu" %% "fapi-client-play30" % "16.0.0-PREVIEW.fpadd-groups-config-field.2025-02-21T1227.15f48131",


Will need to swap out for 16.0.0 once guardian/facia-scala-client#345 is released.

app/model/frontsapi.scala

fronts-client/src/fixtures/shared.ts

github-actions · 2025-02-26T18:02:49Z

Deploy build 8899 of `cms-fronts::facia-tool` to CODE

All deployment options

From guardian/actions-riff-raff.

Fweddi · 2025-02-27T17:19:56Z

public/src/js/models/config/persistence.js

@@ -65,7 +68,8 @@ function flattenModel (model) {
    return _.reduce(model, function (accumulator, value, key) {
        var x = flattenValue(value);

-        if (x) {
+        // We want to drop falsy values, but not 0


Cf. #682

This pr also cleans all falsy values and empty arrays from the meta object to correspond with what is saved in the meta by v1 and avoid any obscure future bugs.

There are a couple of places in the tests where we are explicitly testing falsy values are filtered out, so this seemed the minimum change to allow us to support 0 as a value.

0 as a value is useful as it lets config editors know that a group (e.g. Very Big) has explicitly been set to 0 rather than it is unset and waiting to be filled in.

Fweddi · 2025-02-27T17:20:50Z

public/src/css/style.css

+    display: inline-block;
+}
+
+.capitalize-first-letter:first-letter {


This means we can write 'very big' as 'Very big' (and not 'Very Big' which text-transform: capitalize would do if left to its own devices).

(Stakeholder request)

Fweddi · 2025-02-27T17:25:00Z

It would be good to have future PRs to:

remove all references to groups in place of groupsConfig
allow some groupConfig maxItems to not be editable - specifically, splash, which should only ever be one card*.
fix suppressImages (a boolean) being instantiated with observableNumeric - it works, because observableNumeric is slightly odd, but it shouldn't be instantiated this way

*At least, at this current moment...

davidfurey

👏
I've made a few non-blocking suggestions

davidfurey · 2025-02-27T17:22:05Z

public/src/js/models/config/persistence.js

@@ -65,7 +68,8 @@ function flattenModel (model) {
    return _.reduce(model, function (accumulator, value, key) {
        var x = flattenValue(value);

-        if (x) {
+        // We want to drop falsy values, but not 0
+        if (x || x === 0) {


classic. I'm a big fan of https://typescript-eslint.io/rules/strict-boolean-expressions/

davidfurey · 2025-02-27T17:23:18Z

public/src/js/utils/observable-numeric.js

@@ -8,7 +8,7 @@ export default function(initialValue) {
        },
        write(newValue) {
            var parsedValue = parseFloat(newValue);
-            actual(isNaN(parsedValue) ? newValue : parsedValue);
+            actual(isNaN(parsedValue) ? newValue : Math.abs(parsedValue));


Is the Math.abs required? This is slightly unexpected behaviour for something called "observable-numeric"

It prevents us from modelling negative numbers. This code is invoked in the few cases we expect numbers, and we have already checked at this stage that the variable definitely is a number. Happy to jettison it or move it elsewhere though.

We plan to EOL this app so it is not really a problem for this API to be confusing, let's leave as is.
(with apologies to me or other future developer who gets bitten by this in 5 years time when we are still using this code)

Perhaps we can address this when we fix the suppressImages being treated as an observableNumeric.

davidfurey · 2025-02-27T17:24:40Z

public/src/js/widgets/columns/fronts-config.html

+            <!-- ko if: meta.type() === "flexible/general"-->
+              <div data-bind="foreach: meta.groupsConfig">
+                <label for="flexGenSplashStoryLimit"><span data-bind="text: name" class="capitalize-first-letter"></span> stories</label>
+                <input id="flexGenSplashStoryLimit" type="number" data-bind="value: maxItems" attr: { max: 20, min: 0 } max="20" min="0">


id should be unique, and "SplashStory" in the id is no longer accurate

davidfurey · 2025-02-27T17:33:12Z

It would be good to have future PRs to:

remove all references to groups in place of groupsConfig

allow some groupConfig maxItems to not be editable - specifically, splash, which should only ever be one card*.

fix suppressImages (a boolean) being instantiated with observableNumeric - it works, because observableNumeric is slightly odd, but it shouldn't be instantiated this way

*At least, at this current moment...

When we do 1. we need to make sure that groupsConfig is populated without someone having to go through each container and changing the type backwards/forwards.

For 2., if rendering platforms can handle multi splash cards, JH has no concerns about the tool allowing it. Even if the platforms cannot handle it, he is still fairly relaxed about this.

Fweddi · 2025-02-27T17:41:46Z

It would be good to have future PRs to:

remove all references to groups in place of groupsConfig

allow some groupConfig maxItems to not be editable - specifically, splash, which should only ever be one card*.

fix suppressImages (a boolean) being instantiated with observableNumeric - it works, because observableNumeric is slightly odd, but it shouldn't be instantiated this way

*At least, at this current moment...

When we do 1. we need to make sure that groupsConfig is populated without someone having to go through each container and changing the type backwards/forwards.

For 2., if rendering platforms can handle multi splash cards, JH has no concerns about the tool allowing it. Even if the platforms cannot handle it, he is still fairly relaxed about this.

Thank you for clarifying 2 as I'd expected us to want to keep the hard limit introduced in #1753.

davidfurey · 2025-02-28T09:33:40Z

Thank you for clarifying 2 as I'd expected us to want to keep the hard limit introduced in #1753.

It is fine for the hard-limit to be controlled by configuration, rather than hardcoded.

davidfurey · 2025-02-28T09:42:44Z

public/src/js/widgets/columns/fronts-config.html

+                <label data-bind="attr: { for: name }"><span data-bind="text: name" class="capitalize-first-letter"></span> stories</label>
+                <input type="number" data-bind="value: maxItems, attr: { id: name, max: 20, min: 0 }" max="20" min="0">


I don't think ids can have whitespace. Perhaps this will work (untested):
attr: { for: 'maxItems-' + $index }

prout-bot · 2025-02-28T10:40:08Z

Seen on PROD (merged by @Fweddi 6 minutes and 22 seconds ago) Please check your changes!

Fweddi commented Feb 21, 2025

View reviewed changes

Fweddi force-pushed the fp/max-items-per-group branch from 564bf14 to 9f359da Compare February 21, 2025 14:18

Fweddi commented Feb 21, 2025

View reviewed changes

app/model/frontsapi.scala Show resolved Hide resolved

abeddow91 reviewed Feb 25, 2025

View reviewed changes

fronts-client/src/fixtures/shared.ts Show resolved Hide resolved

Fweddi force-pushed the fp/max-items-per-group branch 2 times, most recently from 740e2ca to 76f9439 Compare February 26, 2025 09:33

Fweddi force-pushed the fp/max-items-per-group branch from e7750a2 to c3c96ff Compare February 27, 2025 09:02

Fweddi changed the title ~~Allow each group to have different maxItems~~ Add editable maxItems field in Config tool Feb 27, 2025

Fweddi force-pushed the fp/max-items-per-group branch 4 times, most recently from b7605a8 to 13fc4ef Compare February 27, 2025 15:09

Fweddi marked this pull request as ready for review February 27, 2025 15:10

Fweddi requested a review from a team as a code owner February 27, 2025 15:10

Fweddi self-assigned this Feb 27, 2025

Fweddi force-pushed the fp/max-items-per-group branch 2 times, most recently from 6cb9177 to f50a18c Compare February 27, 2025 16:28

Fweddi commented Feb 27, 2025

View reviewed changes

davidfurey approved these changes Feb 27, 2025

View reviewed changes

davidfurey reviewed Feb 28, 2025

View reviewed changes

Fweddi added 4 commits February 28, 2025 11:18

Add groupsConfig field

f8e9d8a

Make groupsConfig field editable in /config tool

486acc8

Don't filter out values of 0

f727ed8

Handle binding of knockout observable arrays

1b5cc73

Fweddi added 2 commits February 28, 2025 11:18

Fix typo

440be9c

Convert negative numbers to positive

e3fa482

Fweddi force-pushed the fp/max-items-per-group branch from 8380b3d to 1e2dad6 Compare February 28, 2025 10:18

Set id and label per group

3d0888b

Fweddi force-pushed the fp/max-items-per-group branch from 1e2dad6 to 3d0888b Compare February 28, 2025 10:21

Fweddi merged commit af340ac into main Feb 28, 2025
13 checks passed

Fweddi deleted the fp/max-items-per-group branch February 28, 2025 10:33

prout-bot added Pending-on-PROD Seen-on-PROD and removed Pending-on-PROD labels Feb 28, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add editable maxItems field in Config tool #1763

Add editable maxItems field in Config tool #1763

Fweddi commented Feb 21, 2025 •

edited

Loading

Fweddi Feb 21, 2025

github-actions bot commented Feb 26, 2025 •

edited

Loading

Fweddi Feb 27, 2025

Fweddi Feb 27, 2025

Fweddi Feb 27, 2025

Fweddi commented Feb 27, 2025 •

edited

Loading

davidfurey left a comment

davidfurey Feb 27, 2025

davidfurey Feb 27, 2025

Fweddi Feb 27, 2025

davidfurey Feb 27, 2025

Fweddi Feb 28, 2025

davidfurey Feb 27, 2025

Fweddi Feb 27, 2025

davidfurey commented Feb 27, 2025

Fweddi commented Feb 27, 2025

davidfurey commented Feb 28, 2025

davidfurey Feb 28, 2025

prout-bot commented Feb 28, 2025

		<label data-bind="attr: { for: name }"><span data-bind="text: name" class="capitalize-first-letter"></span> stories</label>
		<input type="number" data-bind="value: maxItems, attr: { id: name, max: 20, min: 0 }" max="20" min="0">

Add editable maxItems field in Config tool #1763

Add editable maxItems field in Config tool #1763

Conversation

Fweddi commented Feb 21, 2025 • edited Loading

What's changed?

Choose a reason for hiding this comment

github-actions bot commented Feb 26, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Fweddi commented Feb 27, 2025 • edited Loading

davidfurey left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davidfurey commented Feb 27, 2025

Fweddi commented Feb 27, 2025

davidfurey commented Feb 28, 2025

Choose a reason for hiding this comment

prout-bot commented Feb 28, 2025

Fweddi commented Feb 21, 2025 •

edited

Loading

github-actions bot commented Feb 26, 2025 •

edited

Loading

Fweddi commented Feb 27, 2025 •

edited

Loading