search

department-of-veterans-affairs / vets-website

Frontend for VA.gov
Creative Commons Zero v1.0 Universal
238 stars 124 forks source link

Determine if it's safe to add the smartbanner.js dependency to vets-website #26518

Closed rmessina1010 closed 7 months ago

rmessina1010 commented 10 months ago

Description

investigate the implementation of a mobile promo banner for non-safari browsers and determine if it's safe and permissible use a third party NPM package via a script tag in content-build.

https://www.npmjs.com/package/smartbanner.js?amp=1

Work done thus far

Confirmed the package is actively maintained Referenced Snyk security to check for vulnerabilities, found no direct vulnerabilities for LSV https://security.snyk.io/vuln/npm?search=smartbanner https://security.snyk.io/package/npm/smartbanner.js/1.21.0

Other Concerns

Confirm this an issue for vets-website as well or will this dependency only affects va.gov-cms?

Relates to

https://github.com/department-of-veterans-affairs/va.gov-cms/issues/15339

jilladams commented 10 months ago

Adding @chriskim2311 for notifications on updates

pjhill commented 10 months ago

Chris put together a draft PR here -- https://github.com/department-of-veterans-affairs/content-build/pull/1774 The banner appears to function without issue with a content-build only change. I don't see a major negative quality impact to the site based on the draft implementation. I don't think there's anything inherently bad about this smartbanner module.

A few things about this to look out for as development proceeds --

  1. Measuring accessibility impact of the banner
  2. Will we be able to tweak contrast ratios and screen reader navigation for the banner?
  3. Are we able to apply VA Design System themes to the banner?

Example page: image

pjhill commented 10 months ago

Oh, I was just reading through the original thread, and I see that Chris has already addressed some a11y concerns -- great!

https://github.com/department-of-veterans-affairs/va.gov-cms/issues/15339#issuecomment-1771854397

pjhill commented 10 months ago

I don't see any reason why this solution shouldn't be pursued from a QA / A11y context.

joeniquette commented 9 months ago

@pjhill Im seeing a lot of errors in staging from a "smartbanner" solution being blocked by our content security policy. Its causing lags in authentication.

jilladams commented 9 months ago

@chriskim2311 FYSA re: Joe's note above: SmartBanner.js appears to be causing auth lags and staging errors from a content security policy. Public Websites introduced that library: ticket: https://github.com/department-of-veterans-affairs/va.gov-cms/issues/15318. PR: https://github.com/department-of-veterans-affairs/content-build/pull/1774

Let us know if you need changes from us. Thanks!