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

Point users to the new form to capture information rather than an email #219

Merged
merged 1 commit into from
Mar 11, 2021

Conversation

jon-betts
Copy link
Contributor

For: #207

This leverages the existing form, and means support get all of their requests through the same medium. This also means support can control the content of the form directly.

Testing notes

@jon-betts jon-betts self-assigned this Mar 9, 2021
@jon-betts
Copy link
Contributor Author

Lint error...

<a href="mailto:[email protected]?subject={{ email_subject | urlencode }}&body={{ email_body | urlencode }}" target="_blank"
>Email us</a>
to let us know you'd like to annotate the page with {{annotated_with}}.
<a href="https://web.hypothes.is/via-request/?subject={{ blocked_url | urlencode }}">Or let us know you'd like to annotate the page with Via.</a>
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
<a href="https://web.hypothes.is/via-request/?subject={{ blocked_url | urlencode }}">Or let us know you'd like to annotate the page with Via.</a>
<a href="https://web.hypothes.is/via-request/?subject={{ blocked_url | urlencode }}">Or let us know you'd like to annotate the page with {{ annotated_with }}.</a>

Copy link
Contributor

@seanh seanh Mar 10, 2021

Choose a reason for hiding this comment

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

👍 to Marcos's suggestion, we can't say "with Via" in the LMS context. I also think it looks nicer if the "or" isn't part of the link:

Suggested change
<a href="https://web.hypothes.is/via-request/?subject={{ blocked_url | urlencode }}">Or let us know you'd like to annotate the page with Via.</a>
Or <a href="https://web.hypothes.is/via-request/?subject={{ blocked_url | urlencode }}">let us know you'd like to annotate the page with {{ annotated_with }}.</a>

@jon-betts Can you make sure this has been tested in the LMS app? The block page and what happens when you click on the link needs to work / make sense in the LMS context as well. For example I suspect that saying Or let us know you'd like to annotate the page with... isn't going to make sense in the LMS app as the Use our browser extension section will have been omitted so the "or" won't make any sense.

The link will have to open in a new tab in LMS too.

There are pre-created localhost assignments for blocked pages in Developer Test Course with Sections Enabled but I don't think they cover all versions of the block pages, so you may have to add some more

Copy link
Contributor

Choose a reason for hiding this comment

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

I think Use our browser extension should just be a <p> now rather than an <h3>. As things stand the Or let us know you'd like to annotate the page with Via text is under the Use our browser extension which doesn't make sense as it's a separate option, not part of using the browser extension
Screenshot from 2021-03-10 12-33-05

Fixed:

Screenshot from 2021-03-10 12-35-41

<a href="mailto:[email protected]?subject={{ email_subject | urlencode }}&body={{ email_body | urlencode }}" target="_blank"
>Email us</a>
to let us know you'd like to annotate the page with {{annotated_with}}.
<a href="https://web.hypothes.is/via-request/?subject={{ blocked_url | urlencode }}">Or let us know you'd like to annotate the page with Via.</a>
Copy link
Contributor

@seanh seanh Mar 10, 2021

Choose a reason for hiding this comment

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

👍 to Marcos's suggestion, we can't say "with Via" in the LMS context. I also think it looks nicer if the "or" isn't part of the link:

Suggested change
<a href="https://web.hypothes.is/via-request/?subject={{ blocked_url | urlencode }}">Or let us know you'd like to annotate the page with Via.</a>
Or <a href="https://web.hypothes.is/via-request/?subject={{ blocked_url | urlencode }}">let us know you'd like to annotate the page with {{ annotated_with }}.</a>

@jon-betts Can you make sure this has been tested in the LMS app? The block page and what happens when you click on the link needs to work / make sense in the LMS context as well. For example I suspect that saying Or let us know you'd like to annotate the page with... isn't going to make sense in the LMS app as the Use our browser extension section will have been omitted so the "or" won't make any sense.

The link will have to open in a new tab in LMS too.

There are pre-created localhost assignments for blocked pages in Developer Test Course with Sections Enabled but I don't think they cover all versions of the block pages, so you may have to add some more

<a href="mailto:[email protected]?subject={{ email_subject | urlencode }}&body={{ email_body | urlencode }}" target="_blank"
>Email us</a>
to let us know you'd like to annotate the page with {{annotated_with}}.
<a href="https://web.hypothes.is/via-request/?subject={{ blocked_url | urlencode }}">Or let us know you'd like to annotate the page with Via.</a>
Copy link
Contributor

Choose a reason for hiding this comment

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

I think Use our browser extension should just be a <p> now rather than an <h3>. As things stand the Or let us know you'd like to annotate the page with Via text is under the Use our browser extension which doesn't make sense as it's a separate option, not part of using the browser extension
Screenshot from 2021-03-10 12-33-05

Fixed:

Screenshot from 2021-03-10 12-35-41

@jon-betts
Copy link
Contributor Author

jon-betts commented Mar 10, 2021

👍 I fixed the annotated_with locally but didn't push. This PR doesn't change the logic of when this message is shown or not, so it wouldn't show up in an LMS context in any case, but it's best to be consistent.

What specifically are we looking to test in the LMS context here? Presumably it's not possible to see this page in LMS as it's only presented for allow list things?

@jon-betts
Copy link
Contributor Author

I've updated with the suggestions made, but I'm not really sure how to test in LMS as suggested. This page is shown when you aren't on the allow list, but we don't apply the allow list in LMS, so there's no way to see the page.

@jon-betts
Copy link
Contributor Author

Hmm, the tests were failing in CI on one of the crypto tests (which this code doesn't touch). I ran it again and it's "fine" now, but that's not particularly encouraging.

This leverages the existing form, and means support get all of their
requests through the same medium. This also means support can control
the content of the form directly.
@jon-betts jon-betts merged commit af558f6 into main Mar 11, 2021
@jon-betts jon-betts deleted the link-to-form branch March 11, 2021 13:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants