Add a support page #9529

eth3lbert · 2024-09-27T06:58:45Z

This PR adds a support page that allows user to select a support type from a list and fill in required information. It also moves "Email Support" from the footer to the support page.

The support page is inspired by npm's support page¹. When a user clicks the "report violation" link, a report form will be displayed. The user must input a crate name, select reasons, and provide detailed information (required when "other" is chosen) before submitting. The name field in the report form should be automatically filled when navigating from the report button in the CrateSidebar.

Related #9496 .
Closes #9478 .

https://www.npmjs.com/support ↩

codecov · 2024-09-27T07:12:55Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 89.11%. Comparing base (c0d0045) to head (ba869e5).
Report is 55 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #9529      +/-   ##
==========================================
- Coverage   89.16%   89.11%   -0.06%     
==========================================
  Files         286      285       -1     
  Lines       29071    28920     -151     
==========================================
- Hits        25922    25772     -150     
+ Misses       3149     3148       -1

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

eth3lbert · 2024-09-27T07:19:57Z

@Turbo87 I've only created the support route for now, and haven't placed it anywhere yet. I believe we want to replace "Email Support" under the "Get Help" section in the footer with this. I'm unsure whether we should simply add a "Get help" mailto link to the categorized support list or differentiate it with a horizontal separator and a subheader like "If you couldn't find a category above." followed by the "Get help" mailto link.

eth3lbert

I've also introduced data-testid="some-id" in this PR instead of the data-test-some-id format. data-testid is becoming more popular recently, which also enables getByTestId in playwright and the potential benefit of removing data-testid in production (further implementation is required) if desired. This comes with a minor drawback of being slightly more verbose in qunit tests.

app/components/support/crate-report-form.hbs

app/controllers/support.js

Turbo87 · 2024-09-27T07:44:40Z

app/controllers/support.js

+export default class SupportController extends Controller {
+  queryParams = ['inquire', 'name'];
+
+  @tracked inquire;


since this is in a controller I think the property keeps its value across page navigations. in other words: if you press the "report crate" button the property will be set, if you navigate away from the page, then you press the support link in the footer, you will not get to the overview state, but to the "report crate" state again. I'm not 100% sure about this, but we should probably add a test to ensure that navigating to /support without query params resets the state.

One more thing I noticed is that, if you're on the support page with query, if the support link in footer is defined as <LinkTo @route="support" /> it will still inherit those query params. There doesn't seem to be a simple way to avoid this inheritance in the template. You need to use hash to manual overwrite all query params.

I'm wondering if there's a way to test this without <LinkTo @route="support"/>. We already have <LinkTo @route="support" @query={{hash inquire=null crate=null }}>Support</LinkTo> that reset the query params by setting their values to null.

not sure about playwright, but in the Ember.js test suite visit('/support') might be sufficient as it does not reload the whole app

Hmm, I just commented out resetController and expected the following to fail, but it didn't 😅 .

test('should not retain query params when exiting and then returning', async function (assert) { await visit('/support?inquire=crate-violation'); assert.strictEqual(currentURL(), '/support?inquire=crate-violation'); assert .dom('[data-test-id="support-main-content"] section') .exists({ count: 1 }) .hasAttribute('data-test-id', 'crate-violation-section'); await visit('/'); assert.strictEqual(currentURL(), '/'); await visit('/support'); assert.strictEqual(currentURL(), '/support'); assert .dom('[data-test-id="support-main-content"] section') .exists({ count: 1 }) .hasAttribute('data-test-id', 'inquire-list-section'); });

However, if there's a <LinkTo @route="support">, I can see that the generated URL has sticky query params.

interesting, maybe I was wrong then :D

I figured out how to test this in qunit but not playwright. I don't know how to mock the route in playwright QQ.

app/components/support/crate-report-form.js

Turbo87 · 2024-09-27T08:30:41Z

the potential benefit of removing data-testid in production

that is already done for data-test-* attributes by https://github.com/mainmatter/ember-test-selectors, so using data-testid and not removing them from prod builds would be a (small) regression for now. we could potentially add data-testid support to ember-test-selectors though, if that is becoming a common pattern in other frameworks.

eth3lbert · 2024-09-27T09:18:27Z

the potential benefit of removing data-testid in production

that is already done for data-test-* attributes by https://github.com/mainmatter/ember-test-selectors, so using data-testid and not removing them from prod builds would be a (small) regression for now. we could potentially add data-testid support to ember-test-selectors though, if that is becoming a common pattern in other frameworks.

Thanks for pointing this out. I didn't know that before.

Yes, I suppose this is a common pattern in newer testing frameworks like testing-library, cypress, and playwright, etc. Do you suggest we stick to the data-test-* convention for now and introduce data-testid later when we implement the removal process for production?

Turbo87 · 2024-09-27T09:54:05Z

Do you suggest we stick to the data-test-* convention for now and introduce data-testid later when we implement the removal process for production?

yeah, I think that would be best for now. I'd rather convert the whole codebase to the other pattern at once, instead of having to deal with two concepts in parallel when possible.

one thing to note, we use things like data-test-crate="serde" too, where the value of the test attribute is dynamic. I'm not sure how that would work with data-testid="crate"? I guess data-testid="crate[serde]" or something like that?

eth3lbert · 2024-09-27T10:11:34Z

one thing to note, we use things like data-test-crate="serde" too, where the value of the test attribute is dynamic. I'm not sure how that would work with data-testid="crate"? I guess data-testid="crate[serde]" or something like that?

Hmm, I'm not sure if I fully understand your question, but I think this can be easily achieved using a template like data-testid="crate-{{crate.name}}".

eth3lbert · 2024-09-27T14:39:09Z

the potential benefit of removing data-testid in production

that is already done for data-test-* attributes by https://github.com/mainmatter/ember-test-selectors, so using data-testid and not removing them from prod builds would be a (small) regression for now. we could potentially add data-testid support to ember-test-selectors though, if that is becoming a common pattern in other frameworks.

I think I've found a possible solution. Since the testid attribute is configurable in Playwright https://playwright.dev/docs/locators#set-a-custom-test-id-attribute, and ember-test-selectors accepts selectors with the pattern data-test-*, we can use data-test-id as the custom testid attribute to satisfy both frameworks.

The custom test id is also compatible with `ember-test-selectors`

The page is inspired by npm's support page.

A report form will be displayed on the support page. The user must input a crate name, select reasons, and provide detailed information (required when the "other" reason is chosen) before submitting.

The report form's crate field should be automatically filled when navigating from the report button in the CrateSidebar.

To display a correct support link (without query params) in the support page, it is necessary to overwrite query params.

Turbo87 · 2024-09-30T13:12:22Z

@eth3lbert is this PR in draft state intentionally or is it ready for review/merging?

eth3lbert · 2024-09-30T14:05:14Z

@eth3lbert is this PR in draft state intentionally or is it ready for review/merging?

Yes, this is almost done. We just need to implement this

I believe we want to replace "Email Support" under the "Get Help" section in the footer with this. I'm unsure whether we should simply add a "Get help" mailto link to the categorized support list or differentiate it with a horizontal separator and a subheader like "If you couldn't find a category above." followed by the "Get help" mailto link.

The simplest and most intuitive way would be to simply place a mailto link after the {{each}} block in template, rather than putting it into supports. I assume that it should always appear after other supports, and when we want to add a new support, we don't need to worry about placing the new support before it. After that, I think it will be ready for review and merging.

Or do you expect this to be done in a separate PR?

Turbo87 · 2024-09-30T14:21:04Z

Or do you expect this to be done in a separate PR?

I'm fine with either path :)

This makes the support link in the footer testable and eliminates the need to manually update query params when new ones are added.

eth3lbert · 2024-09-30T17:27:00Z

I just copied screenshots from Percy for preview. :)

These words need to be revised. :D

Turbo87

eth3lbert commented Sep 27, 2024

View reviewed changes

app/components/support/crate-report-form.hbs Outdated Show resolved Hide resolved

Turbo87 reviewed Sep 27, 2024

View reviewed changes

eth3lbert added 2 commits September 28, 2024 09:49

playwright: Set a custom test id

5d63d65

The custom test id is also compatible with `ember-test-selectors`

frontend: Add a support route

30a689a

The page is inspired by npm's support page.

eth3lbert force-pushed the support-page branch 2 times, most recently from c63f3aa to 85d18ee Compare September 28, 2024 08:11

eth3lbert added 3 commits September 28, 2024 16:20

frontend: Add a crate report form to support page

bed11e3

A report form will be displayed on the support page. The user must input a crate name, select reasons, and provide detailed information (required when the "other" reason is chosen) before submitting.

CrateSidebar: Link report button to support page

892c973

The report form's crate field should be automatically filled when navigating from the report button in the CrateSidebar.

Footer: Add "Support" link

e85a97d

To display a correct support link (without query params) in the support page, it is necessary to overwrite query params.

eth3lbert force-pushed the support-page branch from 85d18ee to e85a97d Compare September 28, 2024 08:20

eth3lbert added 2 commits October 1, 2024 01:01

frontend: Add PristineParamsService

23b904a

This makes the support link in the footer testable and eliminates the need to manually update query params when new ones are added.

Footer: move Email Support to the support page

ba869e5

eth3lbert force-pushed the support-page branch from aa4fe32 to ba869e5 Compare September 30, 2024 17:15

eth3lbert marked this pull request as ready for review September 30, 2024 17:27

eth3lbert requested a review from Turbo87 September 30, 2024 17:27

Turbo87 added C-enhancement ✨ Category: Adding new behavior or a change to the way an existing feature works A-frontend 🐹 labels Oct 1, 2024

Turbo87 approved these changes Oct 1, 2024

View reviewed changes

Turbo87 merged commit 3c970a1 into rust-lang:main Oct 1, 2024
11 checks passed

Turbo87 mentioned this pull request Oct 1, 2024

Report a crate with a form #9496

Closed

eth3lbert deleted the support-page branch October 1, 2024 23:24

Add a support page #9529

Add a support page #9529

Uh oh!

Conversation

eth3lbert commented Sep 27, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Footnotes

Uh oh!

codecov bot commented Sep 27, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

eth3lbert commented Sep 27, 2024

Uh oh!

eth3lbert left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Turbo87 Sep 27, 2024

Choose a reason for hiding this comment

Uh oh!

eth3lbert Sep 27, 2024

Choose a reason for hiding this comment

Uh oh!

eth3lbert Sep 28, 2024

Choose a reason for hiding this comment

Uh oh!

Turbo87 Sep 28, 2024

Choose a reason for hiding this comment

Uh oh!

eth3lbert Sep 28, 2024

Choose a reason for hiding this comment

Uh oh!

Turbo87 Sep 30, 2024

Choose a reason for hiding this comment

Uh oh!

eth3lbert Sep 30, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Turbo87 commented Sep 27, 2024

Uh oh!

eth3lbert commented Sep 27, 2024

Uh oh!

Turbo87 commented Sep 27, 2024

Uh oh!

eth3lbert commented Sep 27, 2024

Uh oh!

eth3lbert commented Sep 27, 2024

Uh oh!

Turbo87 commented Sep 30, 2024

Uh oh!

eth3lbert commented Sep 30, 2024

Uh oh!

Turbo87 commented Sep 30, 2024

Uh oh!

eth3lbert commented Sep 30, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Turbo87 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

eth3lbert commented Sep 27, 2024 •

edited

Loading

codecov bot commented Sep 27, 2024 •

edited

Loading

eth3lbert commented Sep 30, 2024 •

edited

Loading