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

Gc intro block #2229

Merged
merged 15 commits into from
Apr 15, 2024
Merged

Gc intro block #2229

merged 15 commits into from
Apr 15, 2024

Conversation

jmealing
Copy link
Contributor

@jmealing jmealing commented Aug 17, 2023

To do:

[ ✓] GCWeb french
[✓] DTO guidance

DTO design system PRs:
EN: canada-ca/design-system#307
FR: canada-ca/systeme-conception#210

components/gc-intro/gc-intro-block-en.html Outdated Show resolved Hide resolved
components/gc-intro/gc-intro-block-en.html Outdated Show resolved Hide resolved
components/gc-intro/gc-intro-block-en.html Outdated Show resolved Hide resolved
components/gc-intro/gc-intro-block-en.html Outdated Show resolved Hide resolved
components/gc-intro/gc-intro-block-en.html Outdated Show resolved Hide resolved
@jmealing jmealing temporarily deployed to github-ci September 8, 2023 15:52 — with GitHub Actions Inactive
@duboisp
Copy link
Member

duboisp commented Sep 11, 2023

Pre-approved upon successful local testing and review.

@jmealing jmealing temporarily deployed to github-ci September 11, 2023 14:45 — with GitHub Actions Inactive
@jmealing jmealing temporarily deployed to github-ci September 11, 2023 14:52 — with GitHub Actions Inactive
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.

A few changes:

  • The irregular heading structure need to be addressed either by proper messaging or by moving the demo onto its own page.
  • Missing a demo for the first example, I proposed some text and to reuse the current page as being the demo page
  • Should the intro block with the byline added?

Side question: We should think of how this intro pattern are going to be linked with the heading level 1 component (which include double h1).
Is this design pattern are only informational and suggestive? Or will it needs to be monitored like other stable component, to advise web author of any update that must be applied to the intro block if there is a major change?

components/gc-intro/gc-intro-block-en.html Outdated Show resolved Hide resolved
components/gc-intro/gc-intro-block-en.html Outdated Show resolved Hide resolved
components/gc-intro/gc-intro-block-en.html Outdated Show resolved Hide resolved
components/gc-intro/gc-intro-block-en.html Outdated Show resolved Hide resolved
@duboisp
Copy link
Member

duboisp commented Oct 16, 2023

@Garneauma as discussed

@Garneauma
Copy link
Contributor

@jmealing Could you please move this documentation in the newly created section /design-patterns ? Thank you.

@duboisp duboisp added the Query: Project item Part of a github project label Nov 3, 2023
@jmealing
Copy link
Contributor Author

jmealing commented Dec 6, 2023

I can't edit this PR anymore. Not sure why. @delisma ?

Screenshot 2023-12-06 at 1 33 45 PM

Copy link
Contributor

@Garneauma Garneauma left a comment

Choose a reason for hiding this comment

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

  • Please wrap the code sample in a container and add a title for it.
  • Can you convert the documentation to the new data-first approach? You can base your JSON file on the Date modified component.

design-patterns/gc-intro/gc-intro-block-demos-en.html Outdated Show resolved Hide resolved
design-patterns/gc-intro/gc-intro-block-demos-en.html Outdated Show resolved Hide resolved
design-patterns/gc-intro/gc-intro-block-demos-fr.html Outdated Show resolved Hide resolved
design-patterns/gc-intro/gc-intro-block-demos-fr.html 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.

Reviewed and tested, everything is functional and do work as expected. I tested with the feature addition that was made to the background image component.

There is a few request of change, see the inline comments:

  • Remove any reference that leverage our versioning API
  • Make one little translation.

sites/layouts/documentation_pattern.html Outdated Show resolved Hide resolved
sites/layouts/documentation_pattern.html Outdated Show resolved Hide resolved
sites/layouts/documentation_pattern.html Outdated Show resolved Hide resolved
design-patterns/gc-intro/index.json-ld Outdated Show resolved Hide resolved
design-patterns/gc-intro/index.json-ld Outdated Show resolved Hide resolved
design-patterns/gc-intro/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.

One little and tiny change to make. Everything else is good to go.

Side note: If this PR is merged after gc-institution PR, we will simply need to resolve the conflict on the documentation_pattern.html layout.

design-patterns/gc-intro/index.json-ld Outdated Show resolved Hide resolved
duboisp
duboisp previously approved these changes Mar 21, 2024
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 look great.

We will wait for the completion of all the request in https://github.com/orgs/wet-boew/projects/4/views/1 + approval from DTO and PP before to merge it.

@duboisp duboisp removed their assignment Mar 21, 2024
Garneauma
Garneauma previously approved these changes Mar 21, 2024
@Garneauma Garneauma dismissed stale reviews from duboisp and themself via ca2b39c March 27, 2024 15:00
duboisp
duboisp previously approved these changes Apr 3, 2024
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 again and tested again locally,

No issue and this is ready for merge when approved/coordinated with the other related PRs.

@duboisp duboisp removed their assignment Apr 3, 2024
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.

Nothing has changes, this is only resolving the merge conflict regarding our generated files.

So my last review is still accurate and we did get the approval from PP and DTO to merge the PR related to ILP.

@Garneauma Garneauma merged commit 2e80898 into wet-boew:master Apr 15, 2024
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Query: Project item Part of a github project
Projects
Status: Completed
Development

Successfully merging this pull request may close these issues.

4 participants