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.

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

Hide CSAT by default #1566

Merged
merged 1 commit into from
Feb 25, 2022
Merged

Hide CSAT by default #1566

merged 1 commit into from
Feb 25, 2022

Conversation

Vinnl
Copy link
Collaborator

@Vinnl Vinnl commented Feb 23, 2022

This PR fixes #1564.

Only display the CSAT if the script to handle its interactions was
properly loaded (i.e. not blocked by an adblocker).

How to test: Make sure your browser language is set to en-US, is in the US, you don't have RECRUITMENT_BANNER_* env vars set, and you're logged in to an account that has had Premium for at least seven days. (Alternatively, add or True to line 21 of header.html.) You should see the CSAT banner.

Now install/enable an adblocker. You should no longer see the banner. (Whereas before, it would be visible but just not work.)

  • l10n dependencies have been merged, if any.
  • I've added a unit test to test for potential regressions of this bug (or this is a front-end change, where we don't yet have unit test infrastructure).
  • I've added or updated relevant docs in the docs/ directory.
  • All UI revisions follow the coding standards, and use Protocol tokens where applicable (see /static/scss/libs/protocol/css/includes/tokens/dist/index.scss).
  • Commits in this PR are minimal and have descriptive commit messages.

@Vinnl Vinnl self-assigned this Feb 23, 2022
Copy link
Contributor

@maxxcrawford maxxcrawford left a comment

Choose a reason for hiding this comment

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

Approving with a known merge conflict incoming.

If you need to land this before #1545, please add that pesky ?.

return;
}
csatWrapperEl.classList.add("is-visible");
Copy link
Contributor

Choose a reason for hiding this comment

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

Issue: I ran into a bug while testing another PR and added a fix in this commit: 98157cb

Suggestion: We will need to port this over/resolve a merge conflict at some point soon.

Suggested change
csatWrapperEl.classList.add("is-visible");
csatWrapperEl?.classList.add("is-visible");

Copy link
Collaborator Author

@Vinnl Vinnl Feb 25, 2022

Choose a reason for hiding this comment

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

That merge conflict should be fairly easy to resolve, no? 5de9430

Only display the CSAT if the script to handle its interactions was
properly loaded (i.e. not blocked by an adblocker).
@Vinnl Vinnl merged commit aa75531 into main Feb 25, 2022
@Vinnl Vinnl deleted the 1564-csat-error branch February 25, 2022 17:12
@pendantry
Copy link

pendantry commented Feb 26, 2022

Um. I'm not entirely sure how this GitHub thing works, apologies if I'm being premature with this feedback:

Has the issue of this non-working banner been fixed, or is there more to do?

Because I'm still seeing it (it still doesn't work):

image

Notes:

  1. I'm not in the US, I'm in the UK. I don't have English (US) set, I have English (GB) set (because I'm English; go figure).
  2. I've logged out of Firefox Relay, and logged back in again (just in case that might be relevant).
  3. The banner disappears if I disable AdBlockerUltimate:

image

... and it appears if I enable it:

image

Reversed logic somewhere? Or simply a continuation of the grudge against the British Empire (as was)? ;)

@Vinnl
Copy link
Collaborator Author

Vinnl commented Feb 26, 2022

Sorry @pendantry, the fix has been accepted into the main codebase, but it hasn't been published to the main website yet. I believe that when we publish to the main website, that is announced here, but in any case it should land soon-ish.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug] Buttons don't work on 'How satisfied are you with Firefox Relay?' feedback banner
3 participants