Skip to content

GCDS font alignment: update base elements font-size to match GCDS#2475

Merged
Garneauma merged 1 commit intowet-boew:masterfrom
Garneauma:text-size
May 14, 2025
Merged

GCDS font alignment: update base elements font-size to match GCDS#2475
Garneauma merged 1 commit intowet-boew:masterfrom
Garneauma:text-size

Conversation

@Garneauma
Copy link
Copy Markdown
Collaborator

@Garneauma Garneauma commented Feb 20, 2025

(related to wet-517 and wet-525)

@duboisp duboisp changed the title Scaffolding: update base elements font-size to match GCDS GCDS font alignment: update base elements font-size to match GCDS Mar 20, 2025
Copy link
Copy Markdown
Member

@duboisp duboisp left a comment

Choose a reason for hiding this comment

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

After an initial and incomplete review, can you make the following change:

  • Adjust the top/bottom margin for heading/paragraph
  • Change the font resize break point
  • Adjust the font for the language toggle in xsmall view port

@duboisp
Copy link
Copy Markdown
Member

duboisp commented Mar 21, 2025

@Garneauma fyi - We might get some additional guidance from DTO about the adjustment to make. Let's put it on semi-hold.

Note: Regarding the most-requested component, DTO suggested to adjust the size on the column beside instead of shrinking the heading. The "most request" need to remain on a single line when view at 100% in large view port.

Copy link
Copy Markdown
Member

@duboisp duboisp left a comment

Choose a reason for hiding this comment

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

See inline comment about code refactoring optimisation

FYI - Review incomplete, need to do a functional test.

@Garneauma
Copy link
Copy Markdown
Collaborator Author

Pre-approved upon successful review and DTO approval.

Copy link
Copy Markdown
Member

@duboisp duboisp left a comment

Choose a reason for hiding this comment

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

Change required,

See details bellow, commented inline and here a list of modified components to be included in our release note after this PR is merged.

Patch - Common, List, compact - Style - Adjust font size because of alignement with GCDS
-> LGTM
Patch - Common, Scaffolding, output, blockquote, pre - Style - Adjust font size because of alignement with GCDS
-> LGTM but require some code refractoring

Patch - Common, Text level modifier, lead - Style - Adjust font size because of alignement with GCDS
-> LGTM
Patch - Contextual features - Style - Adjust font size and spacing because of alignement with GCDS
-> LGTM
Patch - Most requested - Style - Adjust font size because of alignement with GCDS
-> LGTM
Patch - Service and information - Style - Adjust font size and spacing because of alignement with GCDS
-> LGTM

Provisional feature
* Updated GC Subway
-> LGTM

Patch - Common, Scaffolding, heading (h1-h6) - Style - Adjust font size because of alignement with GCDS
-> LGTM but require some code refractoring
Patch - Common, Scaffolding, Legend - Style - Adjust font size because of alignement with GCDS
-> LGTM
Patch - Common, Form utilities/modifier - Style - Adjust font size because of alignement with GCDS
-> LGTM

Patch - Common, Scaffolder, html, body, main - Style - Adjust font size because of alignement with GCDS
-> LGTM

Patch - Site footer - Style - Adjust font size and spacing because of alignement with GCDS
-> Require change - Re-add deprecated feature, that belong to another PR

Patch - Site header - Style - Adjust font size because of alignement with GCDS
-> LGTM

Patch - Site language - Style - Adjust font size because of alignement with GCDS
-> LGTM

Patch - Template, Application - Style - Adjust font size because of alignement with GCDS
-> LGTM

Patch - Template, Home page, Service and informations - Style - Adjust font size because of alignement with GCDS
-> LGTM

Patch - Template, News - Style - Markup rewrite - Adjust font size because of alignement with GCDS
-> Need to be rethink to allow a smooth transition. We can synchronize that breaking change documented.

Patch - Form validation - Style - Adjust font size because of alignement with GCDS
-> LGTM

duboisp
duboisp previously approved these changes May 9, 2025
Copy link
Copy Markdown
Member

@duboisp duboisp left a comment

Choose a reason for hiding this comment

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

The following 2 impact need to be included, the first one replace an existing entry.

LGTM

  • Patch - Template, News - Semantic - Adjust font size because of alignment with GCDS

  • Patch - Common, Scaffolding, input[placeholder] - Code refactoring

  • Patch - Core - Fixed co-existence font issue with gcds-cards

  • Reviewed all previous concerns and they were all addressed

  • Reviewed and tested the change to the news page template. I tested, review, work as expected and I confirm it's impact was changed from breaking change to a simple patch. Great work!!

  • Version impact - Confirm this is a patch change that touch multiple plugin listed in my 2 reviews.

  • Confirm the co-existence issue with the gcds-card related to the font-size is fixed. Note: There is no demo of that co-existence yet because it will be added only after the narrow pilot if it is successful and when we will began second pilot phase with testing one gcds component broadly on all pages. Other related change are also on its ways.

Copy link
Copy Markdown
Member

@duboisp duboisp left a comment

Choose a reason for hiding this comment

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

Reviewed and tested the change since my last review which consist into updating the color of the line under the h1.

Additional impact

  • Patch - Main page title - Style - Adjust line color because of GCDS alignment

@Garneauma Garneauma merged commit ce65d59 into wet-boew:master May 14, 2025
1 check passed
@duboisp duboisp mentioned this pull request May 15, 2025
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