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 the direct-link CTA for third party accounts #658

Merged
merged 1 commit into from
Jan 30, 2018

Conversation

robertknight
Copy link
Member

The CTA message shown when an anonymous user visits a direct link
contains links to the Hypothesis site and makes other assumptions, such
as the user being able to create accounts and those accounts being free,
which may not be applicable on sites that use third party accounts.

For now, just hide this CTA if the page uses third party accounts.

Fixes #656
Fixes #657


Example of inappropriate message with current released client:

screenshot 2018-01-30 12 20 42

The CTA message shown when an anonymous user visits a direct link
contains links to the Hypothesis site and makes other assumptions, such
as the user being able to create accounts and those accounts being free,
which may not be applicable on sites that use third party accounts.

For now, just hide this CTA if the page uses third party accounts.

Fixes #656
Fixes #657
@codecov
Copy link

codecov bot commented Jan 30, 2018

Codecov Report

Merging #658 into master will increase coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #658      +/-   ##
==========================================
+ Coverage   91.02%   91.03%   +<.01%     
==========================================
  Files         135      135              
  Lines        5394     5397       +3     
  Branches      933      934       +1     
==========================================
+ Hits         4910     4913       +3     
  Misses        484      484
Impacted Files Coverage Δ
src/sidebar/components/sidebar-content.js 89.67% <100%> (+0.2%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b916ee2...ad67c28. Read the comment docs.

@nlisgo
Copy link
Contributor

nlisgo commented Jan 30, 2018

🙏

@seanh
Copy link
Contributor

seanh commented Jan 30, 2018

As we did with another similar banner in the sidebar (Slack discussion, GitHub issue 1, GitHub issue 2) I wonder if we can just remove this banner from the product entirely?

  • We already allow people to try to annotate when not logged in, and when they do that we inform them that they need to log in (the UX of this you-need-to-log-in message is terrible, but that should be fixed, separate problem)

  • We also have a log in button in the top right of the sidebar which, if not prominent enough to be noticed by users, should be made more prominent

As a general rule maybe the UI shouldn't be peppered with additional messages telling users to sign up or log in. @dawariley ?

@dawachan
Copy link

dawachan commented Jan 30, 2018

I've spoken to @seanh about this issue and agreed that we shouldn't surface non applicable messaging to users in the UI, such as in the eLife scenario. However before disabling this CTA to login we need to determine what scenarios this message is displayed in.

I would also add that if there is an additional way to sign in/login provided on the top bar (which according to the screenshot appears to be the case) then it becomes less critical to show this message unless the goal is: to drive H signups or we are concerned that users will miss the login/signup links at the top. Trying to understand what the original rationale was for having this additional messaging display in the first place. Perhaps @dwhly can shed some light on this.
Thanks!

@seanh
Copy link
Contributor

seanh commented Jan 30, 2018

It looks to me as if this notice appears:

  1. Only when you're not logged in
  2. Only when you follow a direct link (e.g. here's one) so that the sidebar is filtered to show one annotation only

@seanh seanh self-assigned this Jan 30, 2018
@seanh
Copy link
Contributor

seanh commented Jan 30, 2018

👍 Tested that the footer still shows in first-party context but no longer shows on publisher test site

@seanh
Copy link
Contributor

seanh commented Jan 30, 2018

LGTM

@seanh seanh merged commit 69a9c2b into master Jan 30, 2018
@seanh seanh deleted the hide-direct-link-cta-for-third-party-accounts branch January 30, 2018 15:42
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.

4 participants