Move favicons to asset pipeline by mitchellhenke · Pull Request #7532 · 18F/identity-idp

mitchellhenke · 2022-12-22T18:36:15Z

🛠 Summary of changes

We currently serve favicon and related assets from the IDP outside of the CDN because they're not in the asset pipeline.

This PR moves them so that we can get all the benefits of caching, fingerprinting, and offloading of IDP requests to the CDN.

jskinne3

Looks great!

aduth · 2022-12-23T17:54:41Z

For the "new" files, are those expected to be moved from public/, not additions?

I also wonder if we could use this as an opportunity to point to assets in the design system, which are already included in the asset pipeline.

For example:

<%= favicon_link_tag(
      design_system_asset_path('img/favicons/apple-touch-icon.png'),
      rel: 'apple-touch-icon',
      sizes: '180x180',
      type: 'image/png'
    ) %>

mitchellhenke · 2022-12-23T18:02:22Z

For the "new" files, are those expected to be moved from public/, not additions?

I left them there mostly as backwards compatibility, but I wasn't sure if it was strictly required. They could be safely deleted once this is deployed though.

I also wonder if we could use this as an opportunity to point to assets in the design system, which are already included in the asset pipeline.

For example:
<%= favicon_link_tag(
      design_system_asset_path('img/favicons/apple-touch-icon.png'),
      rel: 'apple-touch-icon',
      sizes: '180x180',
      type: 'image/png'
    ) %>

Didn't know those were there, I'll give that a go!

aduth · 2022-12-23T18:10:24Z

My concern was mostly about adding new binary data to the repo history (where moving presumably would have avoided any additional download size), but if we can at least point to design system resources in the meantime and not add new images, fine to keep the current public/ assets around 'til after the deploy.

config/initializers/simple_form.rb

changelog: Internal, Assets, Move favicons to asset pipeline

app/views/layouts/base.html.erb

aduth

LGTM 👍

@aduth

* build-sass: Add CLI flag for additional load paths (#7525) changelog: Internal, Build Tooling, Improve Sass compilation tool to support additional load paths * Updating ThreatMetrix failure (sad face) screen content (#7529) * Updating ThreatMetrix failure (sad face) screen content changelog: User Facing Improvements, UX, ThreatMetrix failure screen content * Lints [skipe changelog] * Adding contact center number to config and setting dummy default [skip changelog] * Ignore non-yaml files when normalizing yamls (#7531) [skip changelog] * Lg-8264 po search frontend updates (#7519) * show updated ui for location page when arcgis is enabled * add mile(s) away to distance * changelog: Improvements, Location page, update ui on location pg when arcgis is enabled * normalize yaml * fix missing translation test failure * update strings to include count to handle pluralization * update collection item test * add es translation for tty to untranslated keys * refactor * Fix flakey in-person feature test (#7533) * make it okay for the interstitial to not show it's possible for the interstitial to disappear before the matcher can catch it. changelog: Internal, Automated Testing, Improve reliability of successful automated tests * Make `lintfix` fix erb, javascript, and CSS files too (#7528) * Make lintfix fix erb files too * Autofix javascript and CSS files * [skip changelog] * Mention that we're fixing things in status updates * Fix untranslated email input in password reset (#7537) * Fix untranslated email input in password reset changelog: Bug Fixes, Internationalization, Fix untranslated email input in password reset * fix specs * Update app/views/devise/passwords/new.html.erb Co-authored-by: Andrew Duthie <andrew.duthie@gsa.gov> Co-authored-by: Andrew Duthie <andrew.duthie@gsa.gov> * Move favicons to asset pipeline (#7532) * Move favicons to asset pipeline changelog: Internal, Assets, Move favicons to asset pipeline * use existing design system assets * LG-7489: Remove show_account_recovery_recovery_options Feature flag (#7524) * changelog: Internal, Authentication, Remove show_account_recovery_recovery_options feature flag since feature is permanently turned on * LG-7489: rspec fixes * remove unneeded spec * LG-8089: Update to support notifying user of OTP format in message (#7486) * Improvements, Voice messaging, Notify voice OTP users how long and what type of code to expect (LG-8089) * update voice * translation update * add check to otp_sender * changelog: Improvements, Voice messaging, Notify voice OTP users how long and what type of code to expect * fix spec * Revert "LG-8432: SAML - Update ForceAuthn check (#7514)" (#7520) This reverts commit a7577b6. [skip changelog] * LG-8388: Confirm ThreatMetrix doesn't block IPP (#7521) * LG-8388: Confirm ThreatMetrix doesn't block IPP Spec to confirm that IPP is not blocked by a ThreatMetrix result of 'Reject' [skip changelog] * ensure the threatmetrix profiling is enabled * lints * Update spec/features/idv/in_person_spec.rb Co-authored-by: Andrew Duthie <andrew.duthie@gsa.gov> Co-authored-by: Eric Gade <105373963+eric-gade@users.noreply.github.com> Co-authored-by: Andrew Duthie <andrew.duthie@gsa.gov> * Switching newer Acuant SDK to be the default version (#7513) * Switching newer Acuant SDK to be the default version -- What The introduction of the AB Testing framework for upgrading the Acuant SDK assumes that the "newer" version is the one being tested, and that the older is the "default". This change updates the step doc capture step to assume that version 11.7.1 should be used in all cases when the ab test is switched off. changelog: Internal, Upgrades, Acuant SDK Upgrade * Specifying acuant versions in configs and updating names Specifically, we are switching from use_newer_sdk to use_alternate_sdk changelog: Internal, Upgrades, Acuant SDK Upgrade * Acuant context should use config's acuant version by default changelog: Internal, Upgrades, Acuant SDK Upgrade * Fixing lints [skip changelog] * Revert "Acuant context should use config's acuant version by default" This reverts commit b420998. [skip changelog] * Removing redundant condition (via @aduth) [skip changelog] * Add logging criteria, abstract logging decision into method (#7516) * Abstract logging decision into method * [skip changelog] * lintfix * Test logic in should_log? method * lintfix * Add filename fingerprint to JavaScript locale data script (#7527) * Add filename fingerprint to JavaScript locale data script changelog: Internal, Build Tooling, Invalidate cache for content-only revisions to JavaScript bundles * Update specs * LG-8438: Update language on pages related to OTP (#7515) Update OTP language changelog: Improvements, Multi-Factor Authentication, Update OTP language * Doc auth retry timer should decrease (#7498) * Move timeout calculation to helper * Fix calculation of attempted_at and expires_at * changelog: Bug Fixes, DocAuth, Modify retry timer to decrease * Remove instance variable from helper * Revert helper * Indent Co-authored-by: Mitchell Henke <mitchell.henke@gsa.gov> * build-sass: Add CLI flag for additional load paths (#7525) changelog: Internal, Build Tooling, Improve Sass compilation tool to support additional load paths Co-authored-by: Oren Kanner <oren.kanner@gsa.gov> Co-authored-by: Doug Price <douglas.price@gsa.gov> Co-authored-by: Eric Gade <105373963+eric-gade@users.noreply.github.com> Co-authored-by: Andrew Duthie <andrew.duthie@gsa.gov> Co-authored-by: John Skiles Skinner <john.skinner@gsa.gov> Co-authored-by: jc-gsa <104452882+jc-gsa@users.noreply.github.com> Co-authored-by: Kimball Bighorse <kimball.bighorse@gsa.gov> Co-authored-by: Mitchell Henke <mitchell.henke@gsa.gov> * LG-8414 Remove flag from batch job (#7534) * LG-8414 Remove flag from batch job changelog: Internal, Attempts API, Events Batch work * LG-8390 Write rake tasks to pass or reject users (#7536) * initial steps for rake script to review profile * rewrite tasks for passing and rejecting * add tests for review_profile rake task * add changelog changelog: Internal, IdV, Make script to pass or reject TMX review * activate profile upon passing review also changed the deactivation reason upon rejection to :threatmetrix_review_rejected * check for deactivation reason in passing Co-authored-by: Andrew Duthie <andrew.duthie@gsa.gov> Co-authored-by: Eric Gade <105373963+eric-gade@users.noreply.github.com> Co-authored-by: Sheldon Bachstein <sheldon.bachstein@gsa.gov> Co-authored-by: Shannon A <20867088+svalexander@users.noreply.github.com> Co-authored-by: Tomas Apodaca <thomas.apodaca@gsa.gov> Co-authored-by: Malick Diarra <malick.diarra@gsa.gov> Co-authored-by: Oren Kanner <oren.kanner@gsa.gov> Co-authored-by: Doug Price <douglas.price@gsa.gov> Co-authored-by: John Skiles Skinner <john.skinner@gsa.gov> Co-authored-by: jc-gsa <104452882+jc-gsa@users.noreply.github.com> Co-authored-by: Kimball Bighorse <kimball.bighorse@gsa.gov> Co-authored-by: Luis H. Matos <ThatSpaceGuy@users.noreply.github.com> Co-authored-by: Alex Bradley <alexander.bradley@gsa.gov>

mitchellhenke requested a review from aduth December 22, 2022 18:36

mitchellhenke marked this pull request as ready for review December 22, 2022 19:01

mitchellhenke force-pushed the mitchellhenke/move-favicons-to-asset-pipeline branch from 65adff7 to fb0b3ec Compare December 22, 2022 19:21

jskinne3 approved these changes Dec 22, 2022

View reviewed changes

aduth reviewed Dec 23, 2022

View reviewed changes

config/initializers/simple_form.rb Outdated Show resolved Hide resolved

Move favicons to asset pipeline

49621d3

changelog: Internal, Assets, Move favicons to asset pipeline

mitchellhenke force-pushed the mitchellhenke/move-favicons-to-asset-pipeline branch from b68360d to 672cfda Compare December 23, 2022 18:27

mitchellhenke commented Dec 23, 2022

View reviewed changes

app/views/layouts/base.html.erb Outdated Show resolved Hide resolved

mitchellhenke force-pushed the mitchellhenke/move-favicons-to-asset-pipeline branch 2 times, most recently from dc2b0de to 9719b65 Compare December 23, 2022 18:57

aduth reviewed Dec 23, 2022

View reviewed changes

app/views/layouts/base.html.erb Outdated Show resolved Hide resolved

use existing design system assets

544c8f3

mitchellhenke force-pushed the mitchellhenke/move-favicons-to-asset-pipeline branch from 9719b65 to 544c8f3 Compare December 23, 2022 19:01

aduth approved these changes Dec 23, 2022

View reviewed changes

mitchellhenke merged commit ec27919 into main Dec 23, 2022

mitchellhenke deleted the mitchellhenke/move-favicons-to-asset-pipeline branch December 23, 2022 19:25

mitchellhenke mentioned this pull request Dec 27, 2022

Deploy RC 238 to Production #7542

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move favicons to asset pipeline#7532

Move favicons to asset pipeline#7532
mitchellhenke merged 2 commits intomainfrom
mitchellhenke/move-favicons-to-asset-pipeline

mitchellhenke commented Dec 22, 2022

Uh oh!

jskinne3 left a comment

Uh oh!

aduth commented Dec 23, 2022

Uh oh!

mitchellhenke commented Dec 23, 2022

Uh oh!

aduth commented Dec 23, 2022

Uh oh!

Uh oh!

Uh oh!

Uh oh!

aduth left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

mitchellhenke commented Dec 22, 2022

🛠 Summary of changes

Uh oh!

jskinne3 left a comment

Choose a reason for hiding this comment

Uh oh!

aduth commented Dec 23, 2022

Uh oh!

mitchellhenke commented Dec 23, 2022

Uh oh!

aduth commented Dec 23, 2022

Uh oh!

Uh oh!

Uh oh!

Uh oh!

aduth left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants