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

Add banner to all pages #8579

Merged
merged 11 commits into from
May 18, 2021
Merged

Add banner to all pages #8579

merged 11 commits into from
May 18, 2021

Conversation

lyndaidaii
Copy link
Contributor

@lyndaidaii lyndaidaii commented May 12, 2021

Summary of the changes (in less than 80 characters):

before: banner only show on the home page

after: banner show on all pages
package details page:
displayp
Upload package page:

Addresses #8525

@lyndaidaii lyndaidaii requested a review from a team as a code owner May 12, 2021 19:17
@lyndaidaii lyndaidaii changed the base branch from main to dev May 12, 2021 19:17
@lyndaidaii lyndaidaii changed the title add banner to display, upload, list package pages Add banner to display, upload, list package pages May 12, 2021
@joelverhagen
Copy link
Member

Do we want it on all pages? How did we pick this specific set?

@lyndaidaii
Copy link
Contributor Author

lyndaidaii commented May 12, 2021

Do we want it on all pages? How did we pick this specific set?

yes, we want to it on all pages if this possible. the reason why pick those pages is it has high traffic to thoses pages, especially display package details pages. it has traffic from PMUI or google.
Discussed with Chris, I will add more banner to pages like manage package package for authors

@@ -3,6 +3,7 @@
@{
ViewBag.Title = "Manage My Package";
ViewBag.Tab = "Packages";
ViewBag.DisplayBanner = Model.DisplayBannerFeatureEnabled;
Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am not sure about this approach. if we include in NuGetViewBase, which means flag for banner will be in the configure file instead of feature flag json file. let me know if my thinking is in right track?

@shishirx34
Copy link
Contributor

shishirx34 commented May 12, 2021

Can we make this banner dismissible? It is very annoying to see it constantly for folks who don't care. It was tolerable when it was only on the home page but the user experience is terrible once we enable this on all pages. When asking for survey we should only ask it once when the page is opened after that allow it to be dismissible or it should auto-dismiss i.e. hide it after sometime if the user doesn't click on it, or say user logs in. Even if the user decides to respond to a survey why would we want to show the banner again.

Copy link
Contributor

@shishirx34 shishirx34 left a comment

Choose a reason for hiding this comment

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

I would avoid putting this on all pages, or at least redesign the banner to be dismissible.

@lyndaidaii
Copy link
Contributor Author

lyndaidaii commented May 13, 2021

I would avoid putting this on all pages, or at least redesign the banner to be dismissible.

The reason why we want add to most of pages is because that we are getting small amount of customers' responses this time comparing last time. We want to get more feedback with more banner visible in more pages

@jcjiang
Copy link

jcjiang commented May 13, 2021

@shishirx34 Some more context to not letting the banner be dismissed - the main reason why we decided to sidestep that functionality (for now) is because 1) saving the dismissed state for future sessions from the users would require additional engineering work that given the 2) relatively short windows - usually a week or two - during which the survey is out, might not be worth it.

Completely agree though that as we have surveys that are out for longer or are more visually distracting, we should allow the banner to be dismissed and/or not show on all pages.

@shishirx34
Copy link
Contributor

shishirx34 commented May 13, 2021

@jcjiang - I disagree. Even the period of 1-2 weeks is too long of a period to showcase this anti-pattern. We get millions of views every week on the gallery. I've had personal experience with some users being annoyed when asked to give feedback for not using 2FA after login and this was a one time pop-up. I am certain lots of users will find this annoying. Being persistent and annoying at showing this banner to users just to get the feedback is not a good way to get the data, moreover, it disinclines the users to actually follow through.

I remember that we had some form of survey (HATS?) where we had a "feedback" side rollout pop-up which was a much better way of getting feedback (and this was designed by ux team). This banner is pretty loud with a color scheme that doesn't really go with the site for continuous view and at the top of the page, designed by us with no ux lead or user research. Let's say a user does provide you with the feedback, we still continue to show this banner to that user which isn't a good user experience. I believe this banner was introduced for BLM support and now we are extending, probably misusing, it for survey purpose.

We should either find a different way to get feedback, or at the very least invest engineering effort on making this dismissible so that it is reusable in the future.

We should have a discussion offline if you still disagree.

/cc: @skofman1

@lyndaidaii
Copy link
Contributor Author

lyndaidaii commented May 14, 2021

After discussion, we decide to display banner to all pages now due to reasons following:

  • communication overhead with customers. less confusion for customers since survey banner is alive already.
  • the reason why we are getting low response is not clear, we will investigate more, improve experience after getting more feedback from customers.
  • dismissble button requires some details info(how long banner will be dismiss etc), more engineering/PM time is required, which potentially makes some annoying time for customers.

We will add dismissible button for next NuGet.org survey to improve customer experience.
Created issue for tracking: #8582

Thanks for all input and all considerations!

@lyndaidaii lyndaidaii changed the title Add banner to display, upload, list package pages Add banner to all pages May 14, 2021
cookieExpirationService.Setup(e => e.ExpireAnalyticsCookies(It.IsAny<HttpContextBase>()));
featureFlagsService.Setup(e => e.IsDisplayBannerEnabled()).Returns(false);
Copy link
Contributor

Choose a reason for hiding this comment

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

It's better to have a separate test for the banner flag.

@lyndaidaii lyndaidaii requested a review from zhhyu May 17, 2021 18:54
InvokeOnActionExecutedMethod(controller.ControllerContext, httpContext.Object, controller);

// Assert
Assert.False(controller.ViewBag.CanWriteAnalyticsCookies);
Copy link
Member

Choose a reason for hiding this comment

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

Why assert on analytics cookies? There is already a UT up there to do the same.

Copy link
Member

@joelverhagen joelverhagen left a comment

Choose a reason for hiding this comment

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

Please limit UT assertions to only describe the UT name, i.e. the name of the "fact".
:shipit:

cookieExpirationService.Setup(e => e.ExpireAnalyticsCookies(It.IsAny<HttpContextBase>()));
featureFlagsService.Setup(e => e.IsDisplayBannerEnabled()).Returns(false);
Copy link
Contributor

Choose a reason for hiding this comment

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

There is no need to verify the banner flag in these two tests for cookies, if we have another test to cover it.

// Arrange
var cookieExpirationService = GetMock<ICookieExpirationService>();
var featureFlagsService = GetMock<IFeatureFlagService>();
cookieExpirationService.Setup(e => e.ExpireAnalyticsCookies(It.IsAny<HttpContextBase>()));
Copy link
Contributor

@zhhyu zhhyu May 17, 2021

Choose a reason for hiding this comment

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

You can also remove this setup and verification because we test the banner flag here.

Copy link
Contributor Author

@lyndaidaii lyndaidaii May 18, 2021

Choose a reason for hiding this comment

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

it's private method. I don't think we could remove the set up. let me know if I am wrong.

@lyndaidaii lyndaidaii merged commit 7dc5f3d into dev May 18, 2021
@lyndaidaii lyndaidaii mentioned this pull request May 18, 2021
5 tasks
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.

5 participants