Mntor 1564 - Add Survey Recruitment Link #3030

maxxcrawford · 2023-05-10T17:29:46Z

Summary

This PR adds a survey recruitment link to the top of the page for logged-in/English speaking users.

Acceptance Criteria

Show to en-us only
Show only on the signed in dashboard page
Banner is dismissable

References:

Jira: MNTOR-1564
Figma: N/A

Testing

To show the recruitment link, three items must be true:

Set the following variables in .env:
- RECRUITMENT_BANNER_LINK=https://survey.alchemer.com/s3/6785097/9ee8eccacd7b
- RECRUITMENT_BANNER_TEXT=Tell us about your experience with Firefox Monitor and receive a $100 Amazon gift card.
Set your primary browser language to an English variant (en, en-us, etc).

To "reset" this banner, you'll need to delete a cookie. You can do that by:

Open Web Developer Tools on http://localhost:6060 page
Click on "Storage" tab
Click "Cookies"
Click http://localhost:6060
Remove the recruited cookie

Please reset your app between each test.

Test: Click Survey Link

Run app locally, open homepage in logged out state
Expected: No banner should be visible
Log in. Go to dashboard.
Expected: Banner should be visible
Click on text link "Tell us about…"
Expected: New tab should open to survey link set in ENV
Return to dashboard tab, refresh page
Expected: Banner should no longer be visible

Test: Click Dismiss Link on Survey

Run app locally, open homepage in logged out state
Expected: No banner should be visible
Log in. Go to dashboard.
Expected: Banner should be visible
Click on X/dismiss button on the banner
Expected: Banner should no longer be visible
Refresh page
Expected: Banner should still not be visible

Test: Set Browser to non-English language

Run app locally, open homepage in logged out state
Expected: No banner should be visible
Log in. Go to dashboard.
Expected: Banner should be visible
Set browser to German (de)
Refresh page
Expected: Banner should no longer be visible

Checklist (Definition of Done)

Localization strings (if needed) have been added.
Commits in this PR are minimal and have descriptive commit messages.
I've added or updated the relevant sections in readme and/or code comments
I've added a unit test to test for potential regressions of this bug.
Product Owner accepted the User Story (demo of functionality completed) or waived the privilege.
All acceptance criteria are met.
Jira ticket has been updated (if needed) to match changes made during the development process.
Jira ticket has been updated (if needed) with suggestions for QA when this PR is deployed to stage.

Screenshots

src/client/js/recruitmentSurvey.js

src/views/mainLayout.js

maxxcrawford · 2023-05-10T17:32:53Z

src/views/mainLayout.js

+  }
+
+  const recruitmentBanner = () => ` 
+    <div role="banner" class="recruitment-banner" hidden>


a11y note: Added "banner" type aria role to the element

Hmm, looking at that page, that doesn't seem appropriate for this use case? Our site already has a <header>, which has the same role, and:

each page should generally be limited to a single element with the role of banner.

It also says:

The banner typically includes things such as a logo or corporate identity, or possibly a site-specific search tool, and is generally what your marketing team would call the "header" or "top banner" of the site.

Which I don't think it is.

That said, always 👏 for giving a11y some thought!

Ah, and since it's within that <header> it would even be a banner inside a banner 😅

Removed in ad11567

src/views/mainLayout.js

src/client/css/surveyBanner.css

src/views/mainLayout.js

src/client/css/surveyBanner.css

src/views/mainLayout.js

src/client/css/surveyBanner.css

src/views/mainLayout.js

Vinnl

Looks fine to me, not much to add to Kaitlyn and Peter's comments.

src/client/js/recruitmentSurvey.js

src/client/css/surveyBanner.css

src/views/mainLayout.js

Vinnl · 2023-05-11T08:53:00Z

src/views/mainLayout.js

+  }
+
+  const recruitmentBanner = () => ` 
+    <div role="banner" class="recruitment-banner" hidden>


Hmm, looking at that page, that doesn't seem appropriate for this use case? Our site already has a <header>, which has the same role, and:

each page should generally be limited to a single element with the role of banner.

It also says:

The banner typically includes things such as a logo or corporate identity, or possibly a site-specific search tool, and is generally what your marketing team would call the "header" or "top banner" of the site.

Which I don't think it is.

That said, always 👏 for giving a11y some thought!

Vinnl · 2023-05-11T08:55:52Z

src/views/mainLayout.js

+      <button id="recruitment-dismiss" class="dismiss-btn">
+        <svg class="x-close-icon" role="button" xmlns="http://www.w3.org/2000/svg" viewBox="0 0 20 20"><path d="M14.348 14.849a1.2 1.2 0 0 1-1.697 0L10 11.819l-2.651 3.029a1.2 1.2 0 1 1-1.697-1.697l2.758-3.15-2.759-3.152a1.2 1.2 0 1 1 1.697-1.697L10 8.183l2.651-3.031a1.2 1.2 0 1 1 1.697 1.697l-2.758 3.152 2.758 3.15a1.2 1.2 0 0 1 0 1.698z"/></svg>
+      </button>


This button doesn't currently have a text equivalent (e.g. "Close" or "Dismiss"). Alt text for embedded SVGs are a bit of a hassle (I think you need both an aria-label and a <title> element, although the latter will also make the alt text visible on hover), but perhaps you could add an aria-label to the button?

Added aria-label="Close" to the SVG in ad11567

Just a heads-up that that will need to be localised as well :)

Edit: stole Peter's genius it seems like :)

src/views/mainLayout.js

Vinnl · 2023-05-11T08:59:06Z

src/views/mainLayout.js

+  }
+
+  const recruitmentBanner = () => ` 
+    <div role="banner" class="recruitment-banner" hidden>


Ah, and since it's within that <header> it would even be a banner inside a banner 😅

src/views/mainLayout.js

pdehaan · 2023-05-15T21:17:09Z

src/client/js/recruitmentSurvey.js

+
+  switch (true) {
+    case e.target.matches('#recruitment-banner-dimiss'):
+      document.getElementById('recruitment-banner-link').parentElement.remove()


Q: Do we need to remove the link/button event listeners here as well?

~~I had the same question myself here: #3030 (comment) I'll ask UX and follow-up!~~

Nevermind. I misread your question. Looking into it.

Welp, I'm quoting StackOverflow here:

In modern browsers, you do not need to remove event listeners before removing elements from the DOM. If there are no direct references to that particular DOM element elsewhere in your javascript (e.g. the DOM element reference stored in a JS variable), then the DOM element will be garbage collected after removal. The DOM garbage collector is smart enough to know that an event listener does not count as a reference to that DOM element once it's been removed from the DOM (because DOM events cannot happen when it is removed from the DOM).

I think it's parents' removal is enough to break the references.

Unless the references on L16/L17 are still "references" in memory. I was gonna let QA see what breaks from here.

The only other thing I could do is directly remove the entire banner, rather than the .parentElement bit.

Suggested change

document.getElementById('recruitment-banner-link').parentElement.remove()

recruitmentBanner.remove()

@Vinnl @pdehaan LMK if that is enough answer for y'all. 😅

Vinnl

If you address Peter's comments I think this is good to go by now :)

src/client/css/surveyBanner.css

…nglish-speaking users

Code review suggestions: Updated comment and CSS fix for padding Co-authored-by: Kaitlyn Andres <[email protected]> Co-authored-by: Vincent <[email protected]>

* main: (25 commits) fix: use new error classes from cloud function (#3064) Rename cloud-functions dir, and make missing required env vars warning not error (#3059) Mntor 1539/separate data ingestion (#3034) Merge `main` into `localization` (#3052) Fix MNTOR-1598 - Add wrapper to input form on homepage breach scan (#3045) Mntor 1564 - Add Survey Recruitment Link (#3030) ADR Proposal: Add React Framework (#3019) chore: Remove FluentError chore: Set default error message Delete styleGuide.js add localized text add more descriptive alt attribute for mainlayout MNTOR-1471 - Add missing alt/aria-label attributes on img elements chore: Use enum for the toast type chore: Throw errors with new chore: Omit redundant error message fix: FluentError test chore: Add tests for error classes fix: ClientError import path chore: Remove option to redirect from ClientError ...

maxxcrawford force-pushed the MNTOR-1564-survey-link branch from b778f5a to fc957ce Compare May 10, 2023 17:30

maxxcrawford commented May 10, 2023

View reviewed changes

codemist reviewed May 10, 2023

View reviewed changes

src/client/css/surveyBanner.css Show resolved Hide resolved

codemist reviewed May 10, 2023

View reviewed changes

src/client/css/surveyBanner.css Show resolved Hide resolved

codemist reviewed May 10, 2023

View reviewed changes

src/views/mainLayout.js Outdated Show resolved Hide resolved

codemist reviewed May 10, 2023

View reviewed changes

src/client/css/surveyBanner.css Outdated Show resolved Hide resolved

pdehaan reviewed May 10, 2023

View reviewed changes

Vinnl approved these changes May 11, 2023

View reviewed changes

maxxcrawford mentioned this pull request May 11, 2023

Move all CSS color attributes to variables #3031

Open

pdehaan reviewed May 11, 2023

View reviewed changes

src/views/mainLayout.js Show resolved Hide resolved

pdehaan mentioned this pull request May 11, 2023

Add prettier to pretty the codes #3033

Closed

maxxcrawford mentioned this pull request May 15, 2023

Nav Toggle button may need aria-label #3044

Closed

maxxcrawford requested review from Vinnl, codemist and pdehaan May 15, 2023 20:59

pdehaan reviewed May 15, 2023

View reviewed changes

src/views/mainLayout.js Show resolved Hide resolved

pdehaan reviewed May 15, 2023

View reviewed changes

Vinnl approved these changes May 16, 2023

View reviewed changes

codemist approved these changes May 16, 2023

View reviewed changes

src/client/css/surveyBanner.css Outdated Show resolved Hide resolved

maxxcrawford mentioned this pull request May 16, 2023

Standarize close/dismiss buttons #3046

Closed

maxxcrawford and others added 6 commits May 16, 2023 09:54

Move header elements to a wrapper class to support survey link

24fd3c2

Fix MNTOR-1564: Add logic to conditionally show survey link/text to E…

4000a12

…nglish-speaking users

Apply suggestions from code review

244d7cb

Code review suggestions: Updated comment and CSS fix for padding Co-authored-by: Kaitlyn Andres <[email protected]> Co-authored-by: Vincent <[email protected]>

Update element IDs, refactor logic to show/hide based on one function

4c89ad4

Add countryCode data to logic check for recuritment survey

170eea6

Remove after style on banner

1813bc7

maxxcrawford force-pushed the MNTOR-1564-survey-link branch from f42f37e to 1813bc7 Compare May 16, 2023 14:57

maxxcrawford merged commit a3b927e into main May 16, 2023

maxxcrawford deleted the MNTOR-1564-survey-link branch May 16, 2023 15:35

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Mntor 1564 - Add Survey Recruitment Link #3030

Mntor 1564 - Add Survey Recruitment Link #3030

maxxcrawford commented May 10, 2023

maxxcrawford May 10, 2023

Vinnl May 11, 2023

Vinnl May 11, 2023

maxxcrawford May 11, 2023

maxxcrawford May 15, 2023

Vinnl left a comment

Vinnl May 11, 2023

Vinnl May 11, 2023

maxxcrawford May 15, 2023

Vinnl May 16, 2023 •

edited

Loading

Vinnl May 11, 2023

pdehaan May 15, 2023

maxxcrawford May 16, 2023 •

edited

Loading

maxxcrawford May 16, 2023

maxxcrawford May 16, 2023

maxxcrawford May 16, 2023

Vinnl left a comment

	document.getElementById('recruitment-banner-link').parentElement.remove()
	recruitmentBanner.remove()

Mntor 1564 - Add Survey Recruitment Link #3030

Mntor 1564 - Add Survey Recruitment Link #3030

Conversation

maxxcrawford commented May 10, 2023

Summary

Acceptance Criteria

References:

Testing

Checklist (Definition of Done)

Screenshots

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

Vinnl 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

Vinnl May 16, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maxxcrawford May 16, 2023 • edited Loading

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

Vinnl left a comment

Choose a reason for hiding this comment

Vinnl May 16, 2023 •

edited

Loading

maxxcrawford May 16, 2023 •

edited

Loading