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

All services: Adding new page template and new utilities #2447

Merged
merged 1 commit into from
Dec 5, 2024

Conversation

Garneauma
Copy link
Contributor

@Garneauma Garneauma commented Nov 22, 2024

  • Adding All services page template
  • Adding bg-light utility class to add light-grey background
  • Adding nav-links utility class to style list of navigational links
  • Adding padding and margin utility classes

Changes related to WET-498

Copy link
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.

Incomplete review, just some early comments.

common/list/_lists.scss Outdated Show resolved Hide resolved
common/list/lists-en.html Outdated Show resolved Hide resolved
common/spacing/_spacing.scss Outdated Show resolved Hide resolved
common/spacing/spacing-en.html Show resolved Hide resolved
common/spacing/_spacing.scss Outdated Show resolved Hide resolved
@Garneauma
Copy link
Contributor Author

Pre-approved upon successful review and application of all requested changes.

Copy link
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.

I review and a few change is required, see inline comments.

I only checked the pattern, some fix need to be replicated across the other theme and in French too. I didn't identified all instance. I will check that in a subsequent review.

Note: I carefully reviewed the CSS generated for the padding + margin and it looks good. May be eventually, we should look on providing a working example that show may be not all possibility but at least each aspect of the applied style.

There is a lot of addition in this PR, we will need to be careful to list all of them in our release note. @ouafaaetta head-up

sites/theme.scss Outdated Show resolved Hide resolved
sites/theme.scss Outdated Show resolved Hide resolved
common/list/_lists.scss Outdated Show resolved Hide resolved
common/list/lists-en.html Outdated Show resolved Hide resolved
common/spacing/spacing-en.html Show resolved Hide resolved
templates/all-services/all-services-en.html Outdated Show resolved Hide resolved
templates/all-services/all-services-en.html Outdated Show resolved Hide resolved
templates/all-services/all-services-en.html Outdated Show resolved Hide resolved
templates/all-services/all-services-en.html Outdated Show resolved Hide resolved
templates/all-services/index.json-ld Outdated Show resolved Hide resolved
Copy link
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 my inline comments, still a few edits. Some are more related to content.

common/list/_lists.scss Outdated Show resolved Hide resolved
common/list/lists-en.html Outdated Show resolved Hide resolved
common/list/lists-fr.html Outdated Show resolved Hide resolved
common/spacing/spacing-en.html Show resolved Hide resolved
common/spacing/spacing-en.html Show resolved Hide resolved
templates/all-services/all-services-en.html Outdated Show resolved Hide resolved
templates/all-services/all-services-fr.html Outdated Show resolved Hide resolved
templates/all-services/all-services-fr.html Show resolved Hide resolved
templates/all-services/all-services-fr.html Show resolved Hide resolved
templates/all-services/all-services-fr.html Show resolved Hide resolved
Copy link
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 everything.

Just one thing left and then we will be good to merge.

This is a minor change with all the new utilities added

This is a patch change for the template addition because it is limited to one single page.

common/spacing/spacing-fr.html Outdated Show resolved Hide resolved
@duboisp duboisp merged commit 6c048be into wet-boew:master Dec 5, 2024
1 check passed
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