Verbose option on remainin message summary. For when we need more.#2642
Verbose option on remainin message summary. For when we need more.#2642amazingphilippe wants to merge 2 commits intomainfrom
Conversation
🧪 Review environmenthttps://7nrc5v3msbvh7buw3vqmypsch40uvclc.lambda-url.ca-central-1.on.aws/ |
There was a problem hiding this comment.
Pull request overview
Adds a verbose option to the remaining-messages summary UI so the summary can include the notification unit label (for SMS, “text message parts”) when shown alongside other SMS parts content (notably on “review before sending” screens).
Changes:
- Extend
remaining_messages_summarymacro with averboseparameter to optionally render the notification type label inline. - Update SMS “check before sending” screens to call the macro with
verbose=True. - Add a storybook example and new French translation entries for “text message part(s)”.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| app/translations/csv/fr.csv | Adds French translations for new standalone “text message part(s)” strings. |
| app/templates/views/storybook/remaining-messages-summary.html | Adds a storybook example rendering the summary in verbose mode for SMS. |
| app/templates/views/notifications/check.html | Enables verbose rendering of remaining-messages summary for SMS on the review screen. |
| app/templates/views/check/ok.html | Enables verbose rendering of remaining-messages summary for SMS on the preview screen. |
| app/templates/components/remaining-messages-summary.html | Introduces verbose param and renders notification type label when enabled; changes SMS unit label to “text message parts”. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| <span class="font-bold" data-testid="rms-{{ section.type }}-remaining">{{ section.remaining }}</span> | ||
| {% if verbose %} | ||
| {{ notification_type~" "~section.text }} | ||
| {% else %} | ||
| {{ section.text }} | ||
| {% endif %} |
There was a problem hiding this comment.
The new verbose output concatenates notification_type with the separately-translated section.text ("can be sent before …"). In French, "can be sent before" is currently translated with masculine agreement ("peuvent être envoyés"), but "text message parts" is translated as "parties…" (feminine), producing incorrect grammar when verbose mode is enabled. Consider translating the whole phrase as a single string (with placeholders) or using a gender-neutral French translation for the "can be sent before" fragment to avoid agreement issues.
| <span class="font-bold" data-testid="rms-{{ section.type }}-remaining">{{ section.remaining }}</span> | ||
| {% if verbose %} | ||
| {{ notification_type~" "~section.text }} | ||
| {% else %} | ||
| {{ section.text }} | ||
| {% endif %} |
There was a problem hiding this comment.
In verbose mode the label is always plural (e.g., "text message parts" / "emails"), so cases where section.remaining is 1 will render grammatically incorrect text ("1 … parts"). Since this PR adds both singular and plural translation keys, it would be better to select the correct singular/plural label based on the remaining count for each section.
Summary | Résumé
The summary could be more verbose when adjacent to other "text message parts" content. Such as on the review before sending screens:
Before:
After:
Test instructions | Instructions pour tester la modification