help survey UX updates (#51245)

2025-12-19 18:10:59 -05:00 · 2024-07-10 11:44:57 -07:00
parent 11f9f71f02
commit 2f0b8e1895
6 changed files with 87 additions and 261 deletions
--- a/data/ui.yml
+++ b/data/ui.yml
@@ -56,18 +56,17 @@ support:
  contact_support: Contact support
  ask_community: Ask the GitHub community
 survey:
-  able_to_find: Did this doc help you?
+  able_to_find: Did you find what you needed?
  yes: Yes
  no: No
-  comment_yes_label: Let us know what we do well
-  comment_no_label: Let us know what we can do better
+  cancel: Cancel
+  additional_feedback: Can you tell us more about your rating? (Optional)
  optional: Optional
  required: Required
-  email_placeholder: email@example.com
-  email_label: If we can contact you with more questions, please enter your email address
+  email_placeholder: 'Example: email@example.com'
+  email_label: Leave your email if we can contact you. (Optional)
  email_validation: Please enter a valid email address
  send: Send
-  next: Next
  feedback: Thank you! We received your feedback.
  not_support: If you need a reply, please contact <a href="https://support.github.com/">support</a>.
  privacy_policy: Privacy policy
--- a/src/events/components/Survey.tsx
+++ b/src/events/components/Survey.tsx
@@ -2,8 +2,6 @@ import { useState, useRef, useEffect } from 'react'
 import cx from 'classnames'
 import { useRouter } from 'next/router'
 import { ThumbsdownIcon, ThumbsupIcon } from '@primer/octicons-react'
-import { Spinner } from '@primer/react'
-import useSWR from 'swr'

 import { useTranslation } from 'src/languages/components/useTranslation'
 import { Link } from 'src/frame/components/Link'
@@ -13,7 +11,6 @@ import styles from './Survey.module.scss'

 enum ViewState {
  START = 'START',
-  NEXT = 'NEXT',
  END = 'END',
 }

@@ -27,7 +24,6 @@ type EventData = {
  token?: string
  comment?: string
  email?: string
-  rating?: number
 }

 export const Survey = () => {
@@ -82,63 +78,26 @@ export const Survey = () => {
    }
  }, [email])

-  useEffect(() => {
-    if (state === ViewState.NEXT && data?.comment !== comment.trim()) {
-      setState(ViewState.START)
-      setIsEmailError(false)
-    }
-  }, [comment])
-
-  const { data, error, isLoading } = useSWR(
-    state === ViewState.NEXT && comment.trim() ? '/api/events/survey/preview/v1' : null,
-    async (url: string) => {
-      const response = await fetch(url, {
-        method: 'POST',
-        headers: {
-          Accept: 'application/json',
-          'Content-Type': 'application/json',
-        },
-        body: JSON.stringify({
-          comment: comment.trim(),
-          locale,
-          url: `/${locale}${asPath}`,
-          vote: voteState,
-          token,
-        }),
-      })
-      if (response.ok) {
-        return await response.json()
-      } else {
-        throw new Error(`${response.status} on preview`)
-      }
-    },
-  )
-
-  const hasPreview = !!data && !error && ViewState.NEXT
-
  function submit(evt: React.FormEvent) {
    evt.preventDefault()
    if (voteState === null) return
-    if (hasPreview) {
-      trackEvent(getEventData(voteState === VoteState.YES))
-      if (!isEmailError) {
-        setState(ViewState.END)
-        setIsEmailError(false)
-        setComment('')
-      }
-    } else if (comment.trim()) {
-      setState(ViewState.NEXT)
-    }
+
+    trackEvent(getEventData(voteState === VoteState.YES))
+    completeSurvey()
+  }
+
+  function completeSurvey() {
+    setState(ViewState.END)
+    setIsEmailError(false)
+    setComment('')
  }

  function getEventData(vote: boolean): EventData {
-    const rating = data?.rating
    return {
      vote,
      comment,
      email,
      token,
-      rating,
    }
  }

@@ -161,7 +120,7 @@ export const Survey = () => {
        onChange={(event) => setToken(event.target.value)}
      />

-      {state !== ViewState.END && (
+      {voteState === null && (
        <div className="radio-group mb-2" role="radiogroup" aria-labelledby="survey-title">
          <input
            className={cx(styles.visuallyHidden, styles.customRadio)}
@@ -180,7 +139,7 @@ export const Survey = () => {
            )}
            htmlFor="survey-yes"
          >
-            <span className="visually-hidden">{t`yes`}</span>
+            <span className="mr-2">{t`yes`}</span>
            <ThumbsupIcon
              size={16}
              className={voteState === VoteState.YES ? '' : 'color-fg-muted'}
@@ -203,7 +162,7 @@ export const Survey = () => {
            )}
            htmlFor="survey-no"
          >
-            <span className="visually-hidden">{t`no`}</span>
+            <span className="mr-2">{t`no`}</span>
            <ThumbsdownIcon
              size={16}
              className={voteState === VoteState.NO ? '' : 'color-fg-muted'}
@@ -212,16 +171,19 @@ export const Survey = () => {
        </div>
      )}

-      {error && <span className="f6 color-fg-danger">{t`server_error`}</span>}
+      {state === ViewState.START && voteState && (
+        <p
+          role="status"
+          className="color-fg-muted f6 mb-3"
+          data-testid="survey-end"
+        >{t`feedback`}</p>
+      )}

-      {[ViewState.START, ViewState.NEXT].includes(state) && voteState && (
+      {state === ViewState.START && voteState && (
        <>
          <p className="mb-3">
            <label className="d-block mb-1 f6" htmlFor="survey-comment">
-              <span>
-                {voteState === VoteState.YES && t`comment_yes_label`}
-                {voteState === VoteState.NO && t`comment_no_label`}
-              </span>
+              <span>{t`additional_feedback`}</span>
            </label>
            <textarea
              className="form-control input-sm width-full"
@@ -231,72 +193,51 @@ export const Survey = () => {
              onChange={(event) => setComment(event.target.value)}
            ></textarea>
          </p>
-          {hasPreview && (
-            <div className={cx('form-group', isEmailError ? 'warn' : '')}>
-              <label className="d-block mb-1 f6" htmlFor="survey-email">
-                {t`email_label`}
-                <span className="text-normal color-fg-muted float-right ml-1">{t`optional`}</span>
-              </label>
-              <input
-                type="email"
-                className="form-control input-sm width-full"
-                name="survey-email"
-                id="survey-email"
-                placeholder={t`email_placeholder`}
-                value={email}
-                onChange={(event) => setEmail(event.target.value)}
-                aria-invalid={isEmailError}
-                {...(isEmailError ? { 'aria-describedby': 'email-input-validation' } : {})}
-              />
-              {isEmailError && (
-                <p className="note warning" id="email-input-validation">
-                  {t`email_validation`}
-                </p>
-              )}
-            </div>
-          )}

-          {hasPreview && (
-            <span
-              className="f6 color-fg-muted"
-              dangerouslySetInnerHTML={{ __html: t`not_support` }}
-            ></span>
-          )}
+          <div className={cx('form-group', isEmailError ? 'warn' : '')}>
+            <label className="d-block mb-1 f6" htmlFor="survey-email">
+              {t`email_label`}
+              {/* <p className="text-normal color-fg-muted">{t`email_placeholder`}</p> */}
+            </label>
+            <input
+              type="email"
+              className="form-control input-sm width-full"
+              name="survey-email"
+              id="survey-email"
+              placeholder={t`email_placeholder`}
+              value={email}
+              onChange={(event) => setEmail(event.target.value)}
+              aria-invalid={isEmailError}
+              {...(isEmailError ? { 'aria-describedby': 'email-input-validation' } : {})}
+            />
+            {isEmailError && (
+              <p className="note warning" id="email-input-validation">
+                {t`email_validation`}
+              </p>
+            )}
+          </div>
+
+          <span
+            className="f6 color-fg-muted"
+            dangerouslySetInnerHTML={{ __html: t`not_support` }}
+          ></span>
          <div className="d-flex flex-justify-end flex-items-center mt-3">
            <button
              type="button"
              className="btn btn-sm btn-invisible mr-3"
              onClick={() => {
-                setState(ViewState.START)
-                setVoteState(null)
-                setIsEmailError(false)
+                completeSurvey()
              }}
            >
-              Cancel
+              {t`cancel`}
+            </button>
+            <button
+              disabled={isEmailError}
+              type="submit"
+              className="btn btn-sm color-border-accent-emphasis"
+            >
+              {t`send`}
            </button>
-            {hasPreview ? (
-              <button
-                disabled={isEmailError}
-                type="submit"
-                className="btn btn-sm color-border-accent-emphasis"
-              >
-                {t`send`}
-              </button>
-            ) : (
-              <button
-                type="submit"
-                className="btn btn-sm color-border-accent-emphasis"
-                disabled={isLoading || !comment.trim()}
-              >
-                {t`next`}
-                {isLoading && (
-                  <>
-                    {' '}
-                    <Spinner size="small" />
-                  </>
-                )}
-              </button>
-            )}
          </div>
        </>
      )}
@@ -324,6 +265,5 @@ function trackEvent(eventData: EventData) {
    survey_vote: eventData.vote,
    survey_comment: eventData.comment || undefined,
    survey_email: eventData.email || undefined,
-    survey_rating: eventData.rating,
  })
 }
--- a/src/events/middleware.js
+++ b/src/events/middleware.js
@@ -3,7 +3,6 @@ import { omit, without, mapValues } from 'lodash-es'
 import QuickLRU from 'quick-lru'

 import { schemas, hydroNames } from './lib/schema.js'
-import statsd from '@/observability/lib/statsd.js'
 import catchMiddlewareError from '#src/observability/middleware/catch-middleware-error.js'
 import { noCacheControl } from '#src/frame/middleware/cache-control.js'
 import { getJsonValidator } from '#src/tests/lib/validate-json-schema.js'
@@ -70,6 +69,13 @@ router.post(
      return res.status(400).json(isProd ? {} : validate.errors)
    }

+    if (type === 'survey' && req.body.survey_comment) {
+      req.body.survey_rating = await getSurveyCommentRating({
+        comment: req.body.survey_comment,
+        language: req.body.context.path_language,
+      })
+    }
+
    await publish({
      schema: hydroNames[type],
      value: omit(req.body, OMIT_FIELDS),
@@ -79,26 +85,13 @@ router.post(
  }),
 )

-router.post(
-  '/survey/preview/v1',
-  catchMiddlewareError(async function previewComment(req, res) {
-    noCacheControl(res)
+async function getSurveyCommentRating({ comment, language }) {
+  if (!comment || !comment.trim()) {
+    return
+  }

-    const { comment, locale, url, vote } = req.body
-    const tags = [`url:${url}`, `vote:${vote}`]
-
-    if (!comment || !comment.trim()) {
-      statsd.increment('events.survey_preview.empty', 1, tags)
-      return res.status(400).json({ message: 'Empty comment' })
-    }
-
-    const { rating, signals } = await analyzeComment(comment, locale)
-
-    res.json({ rating, comment })
-
-    tags.push(...signals.map((signal) => `signal:${signal}`))
-    statsd.increment('events.survey_preview.signals', 1, tags)
-  }),
-)
+  const { rating } = await analyzeComment(comment, language)
+  return rating
+}

 export default router
--- a/src/events/tests/middleware.js
+++ b/src/events/tests/middleware.js
@@ -113,56 +113,3 @@ describe('POST /events', () => {
    expect(statusCode).toBe(400)
  })
 })
-
-// These are mostly placeholder tests for now since most of the
-// implementation of this endpoint is not yet written.
-describe('POST /events/survey/preview/v1', () => {
-  test('should respond with 400 when no comment is provided', async () => {
-    const body = JSON.stringify({
-      locale: 'en',
-      url: '/quickstart',
-      vote: 'yes',
-    })
-    const res = await post('/api/events/survey/preview/v1', {
-      body,
-      headers: {
-        'content-type': 'application/json',
-      },
-    })
-    expect(res.statusCode).toBe(400)
-  })
-
-  test('should respond with 400 when comment is provided but empty', async () => {
-    const body = JSON.stringify({
-      locale: 'en',
-      url: '/quickstart',
-      vote: 'yes',
-      comment: '      ',
-    })
-    const res = await post('/api/events/survey/preview/v1', {
-      body,
-      headers: {
-        'content-type': 'application/json',
-      },
-    })
-    expect(res.statusCode).toBe(400)
-  })
-
-  test('should respond with 200 when comment is provided', async () => {
-    const body = JSON.stringify({
-      locale: 'en',
-      url: '/quickstart',
-      vote: 'yes',
-      comment: 'Wonderful',
-    })
-    const res = await post('/api/events/survey/preview/v1', {
-      body,
-      headers: {
-        'content-type': 'application/json',
-      },
-    })
-    const respBody = JSON.parse(res.body)
-    expect(res.statusCode).toBe(200)
-    expect(respBody.rating).toBeLessThanOrEqual(1.0)
-  })
-})
--- a/src/fixtures/fixtures/data/ui.yml
+++ b/src/fixtures/fixtures/data/ui.yml
@@ -56,18 +56,17 @@ support:
  contact_support: Contact support
  ask_community: Ask the GitHub community
 survey:
-  able_to_find: Did this doc help you?
+  able_to_find: Did you find what you needed?
  yes: Yes
  no: No
-  comment_yes_label: Let us know what we do well
-  comment_no_label: Let us know what we can do better
+  cancel: Cancel
+  additional_feedback: Can you tell us more about your rating? (Optional)
  optional: Optional
  required: Required
-  email_placeholder: email@example.com
-  email_label: If we can contact you with more questions, please enter your email address
+  email_placeholder: 'Example: email@example.com'
+  email_label: Leave your email if we can contact you. (Optional)
  email_validation: Please enter a valid email address
  send: Send
-  next: Next
  feedback: Thank you! We received your feedback.
  not_support: If you need a reply, please contact <a href="https://support.github.com/">support</a>.
  privacy_policy: Privacy policy
--- a/src/fixtures/tests/playwright-rendering.spec.ts
+++ b/src/fixtures/tests/playwright-rendering.spec.ts
@@ -500,7 +500,7 @@ test.describe('test nav at different viewports', () => {
 })

 test.describe('survey', () => {
-  test('happy path, thumbs up and enter email', async ({ page }) => {
+  test('happy path, thumbs up and enter comment and email', async ({ page }) => {
    let fulfilled = 0
    // Important to set this up *before* interacting with the page
    // in case of possible race conditions.
@@ -518,17 +518,11 @@ test.describe('survey', () => {

    // The label is visually an SVG. Finding it by its `for` value feels easier.
    await page.locator('[for=survey-yes]').click()
-    await expect(page.getByRole('button', { name: 'Next' })).toBeVisible()
    await expect(page.getByRole('button', { name: 'Cancel' })).toBeVisible()
-    await expect(page.getByRole('button', { name: 'Send' })).not.toBeVisible()
+    await expect(page.getByRole('button', { name: 'Send' })).toBeVisible()

    await page.locator('[for=survey-comment]').click()
    await page.locator('[for=survey-comment]').fill('This is a comment')
-    await page.getByRole('button', { name: 'Next' }).click()
-    await expect(page.getByRole('button', { name: 'Cancel' })).toBeVisible()
-    await expect(page.getByRole('button', { name: 'Send' })).toBeVisible()
-    await expect(page.getByRole('button', { name: 'Next' })).not.toBeVisible()
-
    await page.getByPlaceholder('email@example.com').click()
    await page.getByPlaceholder('email@example.com').fill('test@example.com')
    await page.getByRole('button', { name: 'Send' }).click()
@@ -538,7 +532,7 @@ test.describe('survey', () => {
    await expect(page.getByTestId('survey-end')).toBeVisible()
  })

-  test('thumbs down without filling in the form sends an API POST', async ({ page }) => {
+  test('thumbs up without filling in the form sends an API POST', async ({ page }) => {
    let fulfilled = 0
    // Important to set this up *before* interacting with the page
    // in case of possible race conditions.
@@ -558,7 +552,7 @@ test.describe('survey', () => {
    // One for the page view event and one for the thumbs up click
    expect(fulfilled).toBe(1 + 1)

-    await expect(page.getByRole('button', { name: 'Next' })).toBeVisible()
+    await expect(page.getByRole('button', { name: 'Send' })).toBeVisible()
    await page.getByRole('button', { name: 'Cancel' }).click()
  })

@@ -573,59 +567,13 @@ test.describe('survey', () => {

    await expect(page.locator('[for=survey-comment]')).not.toBeVisible()
    await page.locator('[for=survey-yes]').click()
-    await expect(page.getByRole('button', { name: 'Next' })).toBeVisible()
+    await expect(page.getByRole('button', { name: 'Send' })).toBeVisible()
    await expect(page.locator('[for=survey-comment]')).toBeVisible()

    await page.getByTestId('product-sidebar').getByLabel('Bar', { exact: true }).click()
+    await expect(page.getByRole('button', { name: 'Send' })).not.toBeVisible()
    await expect(page.locator('[for=survey-comment]')).not.toBeVisible()
  })
-
-  test('add survey comment, then modify the survey comment', async ({ page }) => {
-    let fulfilled = 0
-    // Important to set this up *before* interacting with the page
-    // in case of possible race conditions.
-    await page.route('**/api/events', (route, request) => {
-      route.fulfill({})
-      expect(request.method()).toBe('POST')
-      fulfilled++
-      // At the time of writing you can't get the posted payload
-      // when you use `navigator.sendBeacon(url, data)`.
-      // So we can't make assertions about the payload.
-      // See https://github.com/microsoft/playwright/issues/12231
-    })
-
-    await page.goto('/get-started/foo/for-playwright')
-
-    // The label is visually an SVG. Finding it by its `for` value feels easier.
-    await page.locator('[for=survey-yes]').click()
-    await expect(page.getByRole('button', { name: 'Next' })).toBeVisible()
-    await expect(page.getByRole('button', { name: 'Send' })).not.toBeVisible()
-
-    await page.locator('[for=survey-comment]').click()
-    await page.locator('[for=survey-comment]').fill('This is a comment')
-    await page.getByRole('button', { name: 'Next' }).click()
-    await expect(page.getByRole('button', { name: 'Cancel' })).toBeVisible()
-    await expect(page.getByRole('button', { name: 'Send' })).toBeVisible()
-    await expect(page.getByRole('button', { name: 'Next' })).not.toBeVisible()
-
-    await page.locator('[for=survey-comment]').click()
-    await page.locator('[for=survey-comment]').fill('This is a modified comment')
-    // The "Send" button should no longer be visible
-    // and the "Next" button should be visible
-    await expect(page.getByRole('button', { name: 'Cancel' })).toBeVisible()
-    await expect(page.getByRole('button', { name: 'Send' })).not.toBeVisible()
-    await expect(page.getByRole('button', { name: 'Next' })).toBeVisible()
-    await page.getByRole('button', { name: 'Next' }).click()
-    await expect(page.getByRole('button', { name: 'Cancel' })).toBeVisible()
-    await expect(page.getByRole('button', { name: 'Send' })).toBeVisible()
-    await expect(page.getByRole('button', { name: 'Next' })).not.toBeVisible()
-
-    await page.getByRole('button', { name: 'Send' }).click()
-    // One for the page view event, one for the thumbs up click, one for
-    // the submission.
-    expect(fulfilled).toBe(1 + 2)
-    await expect(page.getByTestId('survey-end')).toBeVisible()
-  })
 })

 test.describe('rest API reference pages', () => {