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.

Sign up for GitHub

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

Jump to bottom

Discovery/load gcds #2048

Closed
wants to merge 6 commits into from
Closed

Discovery/load gcds #2048

wants to merge 6 commits into from

Conversation

amazingphilippe
Copy link
Contributor

@amazingphilippe amazingphilippe commented Jan 13, 2025 •
edited
Loading

Summary | Résumé

Configured the gulp pipeline to copy GCDS styles and scripts over to our static folder.

  • I also needed to add headers since GCDS loads fontawesome from cloudflare. Should they be more precise? I feel like its bad that we allow any css and js from cloudflare...
  • I also removed some series() directives in gulp. They were useless the way they were configured.

Test instructions | Instructions pour tester la modification

Sequential steps (1., 2., 3., ...) that describe how to test this change. This
will help a developer test things out without too much detective work. Also,
include any environmental setup steps that aren't in the normal README steps
and/or any time-based elements that this requires.

Étapes consécutives (1., 2., 3., …) qui décrivent la façon de tester la
modification. Elles aideront les développeurs à faire des tests sans avoir à
jouer au détective. Veuillez aussi inclure toutes les étapes de configuration
de l’environnement qui ne font pas partie des étapes normales dans le fichier
README et tout élément temporel requis.

github-advanced-security[bot]
github-advanced-security bot found potential problems Jan 13, 2025
View reviewed changes
gulpfile.js Fixed Show fixed Hide fixed
gulpfile.js Fixed Show fixed Hide fixed
@github-actions GitHub Actions
Copy link

🧪 Review environment

https://iimmeikavkwger5cs4i3nmavfa0ryced.lambda-url.ca-central-1.on.aws/

andrewleith
andrewleith reviewed Jan 14, 2025
View reviewed changes
Copy link
Member

@andrewleith andrewleith left a comment

Choose a reason for hiding this comment

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

This looks good to me. Probably wouldn't merge it in just yet though.

Maybe we can use this as a base to try out some components and just make sure there are no incompatibilities with the rest of the app?

app/templates/main_template.html
Comment on lines +15 to +20
<!-- GCDS icons -->
<link rel="stylesheet" href="https://cdnjs.cloudflare.com/ajax/libs/font-awesome/6.4.2/css/all.min.css"
crossorigin="anonymous">

<!-- GC Design System -->
<link rel="stylesheet" media="screen" href="{{ asset_url('stylesheets/gcds.css') }}" />
Copy link
Member

Choose a reason for hiding this comment

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

I wonder why we need to load 2 separate CSS files here - it would be nice if these were consolidated.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think they'll be removing fontawesome soon in favour of their own icons.

app/templates/main_template.html
@@ -12,6 +12,16 @@
{% include 'partials/google-tag-manager-head.html' %}
{% include 'partials/qualtrics-head.html' %}

<!-- GCDS icons -->
<link rel="stylesheet" href="https://cdnjs.cloudflare.com/ajax/libs/font-awesome/6.4.2/css/all.min.css"
Copy link
Member

Choose a reason for hiding this comment

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

One thing to note here is that we are likely loading font-awesome twice now. Once here for GCDS, and then again for notify itself.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We have our build pipeline to import a subset at least. So some icons will load twice for sure ... 😞

@amazingphilippe
Copy link
Contributor Author

amazingphilippe commented Jan 14, 2025 •
edited
Loading

wouldn't merge it in just yet though.

Oh yes! I don't know why I did a PR, What I wanted to do is to start our discoveries from a common branch. I'll close this!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@andrewleith andrewleith andrewleith left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@amazingphilippe @andrewleith