Banners 9223 #12959

kelsonic · 2020-06-04T17:47:54Z

Description

Issue: department-of-veterans-affairs/va.gov-team#9223

This PR implements the first version of a new site-wide banners management folder. This folder currently contains the Maintenance Banner and the Homepage Banner (aka Emergency Banner). It may include other banners in the future should the need arise.

Testing done

Created unit + e2e tests. The e2e tests don't run currently as we need to have the ability to change the system clock the e2e test suite runs in to test this code with Nightwatch.

Screenshots

Before

Pre-Downtime (defaults to 12 hours before `startsAt`) (1+ days of downtime)

Pre-Downtime (defaults to 12 hours before `startsAt`) (less than 1 day of downtime)

During Downtime (1+ days of downtime)

During Downtime (less than 1 day of downtime)

After

What about mobile?

Acceptance criteria

Implement first version of system maintenance banner.

Definition of done

Events are logged appropriately
Documentation has been updated, if applicable
A link has been provided to the originating GitHub issue (or connected to it via ZenHub)
No sensitive information (i.e. PII/credentials/internal URLs/etc.) is captured in logging, hardcoded, or specs

src/platform/site-wide/maintenance-banner/tests/01-accessibility.e2e.spec.js

src/platform/site-wide/maintenance-banner/tests/components/App.unit.spec.js

src/site/includes/header.html

ncksllvn

Loving it!

src/platform/site-wide/maintenance-banner/components/App.js

src/platform/site-wide/maintenance-banner/config.js

src/site/includes/header.html

src/platform/site-wide/maintenance-banner/tests/01-accessibility.e2e.spec.js

rianfowler · 2020-06-05T15:21:54Z

src/platform/site-wide/maintenance-banner/components/App.js

+const {
+  content,
+  expiresAt,
+  id,
+  startsAt,
+  title,
+  warnContent,
+  warnStartsAt,
+  warnTitle,
+} = config;


It would be more idiomatic to pass these in as props.

We should fetch this information from pager duty using something like getCurrentGlobalDowntime in src/platform/monitoring/DowntimeNotification/util/helpers.js. The parent component for this could transform that data into the view props (e.g. I think you only get the downtime window but you'd need to turn that into stuff like warnStartsAt).

If we use that helper then we don't need to do a site deploy to turn this on- we can set a maintenance window for the global downtime in PagerDuty.

Oh nice! I'll check that out 🙂

va-vfs-bot

ESLint is disabled

vets-website uses ESLint to help enforce code quality. In most situations we would like ESLint to remain enabled.

What you can do

See if the code can be refactored to avoid disabling ESLint, or wait for a VSP review.

src/platform/site-wide/banners/components/HomepageBanner/index.js

ncksllvn

Fantastic work! 👏

src/platform/site-wide/banners/components/Banners/index.js

src/applications/static-pages/homepage-banner/HomepageBanner.jsx

src/site/includes/header.html

ncksllvn · 2020-06-05T21:25:59Z

src/platform/site-wide/banners/components/MaintenanceBanner/index.js

+    id: PropTypes.string.isRequired,
+    startsAt: PropTypes.object.isRequired,
+    title: PropTypes.string.isRequired,
+    warnContent: PropTypes.string,


I think we were asked to add some enforcement for the 300-length limit on the content so maybe we could throw an explicit error if the text length exceeds that. That could make sense as a follow-up PR though and to ahve this lay the foundation

Oh you're right! As the timestamp changes that length will change, too 🤔 Hmm...

Yeah, and the length also doesn't include HTML tags (only the text content itself) so it's not actually a super easy problem

Ooo, do you think I should set inner html here? I wasn't planning on the maintenance banner having jsx elements in it other than just a plain string with formatted startsAt, etc. Currently it just shows this non-html string.

Good question. I'm a little torn because on one hand we want to standardize the messaging but we also want to cap the message at a 300-character limit, which suggests we do intend to customize the message every now and then. How you have it now as a plain string seems totally fine as a starting point

The maintenance banner content shouldn't change after this implementation (theoretically of course...), so isn't putting a length limit on it a little overkill if it's not going to change?

I could see a length limit being useful for the homepage banner

src/applications/static-pages/homepage-banner/HomepageBanner.jsx

src/platform/site-wide/banners/components/HomepageBanner/index.unit.spec.js

src/platform/site-wide/banners/index.js

kelsonic · 2020-06-10T19:16:23Z

@department-of-veterans-affairs/frontend-review-group ready for review! 🎉

kelsonic · 2020-06-12T16:27:52Z

Can't seem to figure out which unit tests are failing here, they all seem to pass locally 🤔 http://jenkins.vfs.va.gov/blue/organizations/jenkins/testing%2Fvets-website/detail/banners-9223/28/tests

kelsonic added front end VSA labels Jun 4, 2020

kelsonic requested review from ncksllvn and a team June 4, 2020 17:47

kelsonic requested a review from a team as a code owner June 4, 2020 17:47

kelsonic self-assigned this Jun 4, 2020

kelsonic commented Jun 4, 2020

View reviewed changes

src/platform/site-wide/maintenance-banner/tests/01-accessibility.e2e.spec.js Outdated Show resolved Hide resolved

kelsonic commented Jun 4, 2020

View reviewed changes

src/platform/site-wide/maintenance-banner/tests/components/App.unit.spec.js Outdated Show resolved Hide resolved

kelsonic commented Jun 4, 2020

View reviewed changes

src/site/includes/header.html Outdated Show resolved Hide resolved

ncksllvn reviewed Jun 5, 2020

View reviewed changes

rianfowler reviewed Jun 5, 2020

View reviewed changes

va-vfs-bot requested a review from a team June 5, 2020 20:38

va-vfs-bot reviewed Jun 5, 2020

View reviewed changes

src/platform/site-wide/banners/components/HomepageBanner/index.js Show resolved Hide resolved

ncksllvn approved these changes Jun 5, 2020

View reviewed changes

va-vfs-bot temporarily deployed to master/banners-9223 June 8, 2020 19:42 Inactive

bkjohnson reviewed Jun 8, 2020

View reviewed changes

src/applications/static-pages/homepage-banner/HomepageBanner.jsx Show resolved Hide resolved

src/platform/site-wide/banners/components/HomepageBanner/index.unit.spec.js Outdated Show resolved Hide resolved

src/platform/site-wide/banners/index.js Outdated Show resolved Hide resolved

va-vfs-bot temporarily deployed to master/banners-9223 June 8, 2020 23:18 Inactive

kelsonic force-pushed the banners-9223 branch from 0999c8c to fc531d9 Compare June 9, 2020 13:53

va-vfs-bot temporarily deployed to master/banners-9223 June 9, 2020 14:24 Inactive

kelsonic force-pushed the banners-9223 branch from fc531d9 to 50f2c5d Compare June 9, 2020 16:42

va-vfs-bot temporarily deployed to master/banners-9223 June 9, 2020 17:06 Inactive

va-vfs-bot temporarily deployed to master/banners-9223 June 9, 2020 20:48 Inactive

kelsonic force-pushed the banners-9223 branch from 01dbe7c to a43d857 Compare June 10, 2020 14:26

va-vfs-bot temporarily deployed to master/banners-9223 June 10, 2020 14:51 Inactive

kelsonic force-pushed the banners-9223 branch from 4aa9711 to fc42eb1 Compare June 11, 2020 17:46

kelsonic mentioned this pull request Jun 11, 2020

Sitewide System Maintenance Banner department-of-veterans-affairs/va.gov-team#9223

Closed

8 tasks

kelsonic force-pushed the banners-9223 branch from 7f5d8ba to dd97ef4 Compare June 11, 2020 19:43

rianfowler approved these changes Jun 11, 2020

View reviewed changes

Nick Sullivan and others added 21 commits June 15, 2020 09:20

WIP

8bea6a7

Delete homepage banner liquid template

98b770c

Add maintenance banner component boilerplate

a49180e

Add POC for maintenance banner

687fe9f

Fix linting issue

4efe268

Update config to not show maintenance-banner currently

75503ee

Update formatting of config for readability

d9fea73

Add Banners component that contains Homepage and Maintenance banners

d43ddff

Remove unnecessary homepage banner

f13f1ba

Fix unit tests

43cb4da

Add disabled e2e tests

12d2c42

Remove unused unit test

a212ed9

Remove sanitize html as dep

06153c2

Update unit test name

f2c18cc

Update comment

3bfb1a6

Update content

3639bdc

Add support for ET on displayed dates

1a3e11f

Update styling

20d98c5

Add max-width: 650px to inner content

7972885

Minor tweaks

3948edc

Move styles import to app entry

04688b9

kelsonic force-pushed the banners-9223 branch from 34d6880 to 04688b9 Compare June 15, 2020 15:20

va-vfs-bot temporarily deployed to master/banners-9223 June 15, 2020 15:43 Inactive

kelsonic merged commit 71cc549 into master Jun 15, 2020

kelsonic deleted the banners-9223 branch June 15, 2020 15:46

kelsonic mentioned this pull request Jun 15, 2020

Integrate maintenance banner with pager duty department-of-veterans-affairs/va.gov-team#10234

Closed

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Banners 9223 #12959

Banners 9223 #12959

kelsonic commented Jun 4, 2020 •

edited

Loading

ncksllvn left a comment

rianfowler Jun 5, 2020

kelsonic Jun 5, 2020

va-vfs-bot left a comment

ncksllvn left a comment

ncksllvn Jun 5, 2020

kelsonic Jun 8, 2020

ncksllvn Jun 8, 2020

kelsonic Jun 8, 2020

ncksllvn Jun 9, 2020

kelsonic Jun 10, 2020

kelsonic Jun 10, 2020

kelsonic commented Jun 10, 2020

kelsonic commented Jun 12, 2020

Banners 9223 #12959

Banners 9223 #12959

Conversation

kelsonic commented Jun 4, 2020 • edited Loading

Description

Testing done

Screenshots

Before

Pre-Downtime (defaults to 12 hours before startsAt) (1+ days of downtime)

Pre-Downtime (defaults to 12 hours before startsAt) (less than 1 day of downtime)

During Downtime (1+ days of downtime)

During Downtime (less than 1 day of downtime)

After

What about mobile?

Acceptance criteria

Definition of done

ncksllvn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

va-vfs-bot left a comment

Choose a reason for hiding this comment

ESLint is disabled

What you can do

ncksllvn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kelsonic commented Jun 10, 2020

kelsonic commented Jun 12, 2020

kelsonic commented Jun 4, 2020 •

edited

Loading

Pre-Downtime (defaults to 12 hours before `startsAt`) (1+ days of downtime)

Pre-Downtime (defaults to 12 hours before `startsAt`) (less than 1 day of downtime)