Skip to content

Commit e63d5c9

Browse files
authored
Merge pull request #33460 from github/repo-sync
Repo sync
2 parents 186fe3c + 3b75d51 commit e63d5c9

File tree

6 files changed

+113
-2
lines changed

6 files changed

+113
-2
lines changed

src/events/components/Survey.tsx

Lines changed: 8 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -82,6 +82,13 @@ export const Survey = () => {
8282
}
8383
}, [email])
8484

85+
useEffect(() => {
86+
if (state === ViewState.NEXT && data?.comment !== comment.trim()) {
87+
setState(ViewState.START)
88+
setIsEmailError(false)
89+
}
90+
}, [comment])
91+
8592
const { data, error, isLoading } = useSWR(
8693
state === ViewState.NEXT && comment.trim() ? '/api/events/survey/preview/v1' : null,
8794
async (url: string) => {
@@ -107,7 +114,7 @@ export const Survey = () => {
107114
},
108115
)
109116

110-
const hasPreview = !!data && !error
117+
const hasPreview = !!data && !error && ViewState.NEXT
111118

112119
function submit(evt: React.FormEvent) {
113120
evt.preventDefault()

src/events/middleware.js

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -94,7 +94,7 @@ router.post(
9494

9595
const { rating, signals } = await analyzeComment(comment, locale)
9696

97-
res.json({ rating })
97+
res.json({ rating, comment })
9898

9999
tags.push(...signals.map((signal) => `signal:${signal}`))
100100
statsd.increment('events.survey_preview.signals', 1, tags)

src/fixtures/fixtures/translations/ja-jp/content/get-started/start-your-journey/hello-world.md

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -23,3 +23,11 @@ recover from it.
2323
## Use of a reusable that might have auto-title links
2424

2525
{% data reusables.gated-features.more-info %}
26+
27+
If you type in "bar" into Google Translate (English to Japanese)
28+
you get "バー". The translation guidelines says that when you're
29+
mentioning an English word, you should leave it and translate it
30+
using the same character composition used for regular Markdown
31+
links. Here's one such example:
32+
33+
[[Bar](バー)](/get-started/foo/bar)

src/fixtures/tests/playwright-rendering.spec.ts

Lines changed: 47 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -579,6 +579,53 @@ test.describe('survey', () => {
579579
await page.getByTestId('product-sidebar').getByLabel('Bar', { exact: true }).click()
580580
await expect(page.locator('[for=survey-comment]')).not.toBeVisible()
581581
})
582+
583+
test('add survey comment, then modify the survey comment', async ({ page }) => {
584+
let fulfilled = 0
585+
// Important to set this up *before* interacting with the page
586+
// in case of possible race conditions.
587+
await page.route('**/api/events', (route, request) => {
588+
route.fulfill({})
589+
expect(request.method()).toBe('POST')
590+
fulfilled++
591+
// At the time of writing you can't get the posted payload
592+
// when you use `navigator.sendBeacon(url, data)`.
593+
// So we can't make assertions about the payload.
594+
// See https://github.com/microsoft/playwright/issues/12231
595+
})
596+
597+
await page.goto('/get-started/foo/for-playwright')
598+
599+
// The label is visually an SVG. Finding it by its `for` value feels easier.
600+
await page.locator('[for=survey-yes]').click()
601+
await expect(page.getByRole('button', { name: 'Next' })).toBeVisible()
602+
await expect(page.getByRole('button', { name: 'Send' })).not.toBeVisible()
603+
604+
await page.locator('[for=survey-comment]').click()
605+
await page.locator('[for=survey-comment]').fill('This is a comment')
606+
await page.getByRole('button', { name: 'Next' }).click()
607+
await expect(page.getByRole('button', { name: 'Cancel' })).toBeVisible()
608+
await expect(page.getByRole('button', { name: 'Send' })).toBeVisible()
609+
await expect(page.getByRole('button', { name: 'Next' })).not.toBeVisible()
610+
611+
await page.locator('[for=survey-comment]').click()
612+
await page.locator('[for=survey-comment]').fill('This is a modified comment')
613+
// The "Send" button should no longer be visible
614+
// and the "Next" button should be visible
615+
await expect(page.getByRole('button', { name: 'Cancel' })).toBeVisible()
616+
await expect(page.getByRole('button', { name: 'Send' })).not.toBeVisible()
617+
await expect(page.getByRole('button', { name: 'Next' })).toBeVisible()
618+
await page.getByRole('button', { name: 'Next' }).click()
619+
await expect(page.getByRole('button', { name: 'Cancel' })).toBeVisible()
620+
await expect(page.getByRole('button', { name: 'Send' })).toBeVisible()
621+
await expect(page.getByRole('button', { name: 'Next' })).not.toBeVisible()
622+
623+
await page.getByRole('button', { name: 'Send' }).click()
624+
// One for the page view event, one for the thumbs up click, one for
625+
// the submission.
626+
expect(fulfilled).toBe(1 + 2)
627+
await expect(page.getByTestId('survey-end')).toBeVisible()
628+
})
582629
})
583630

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

src/fixtures/tests/translations.js

Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -103,4 +103,21 @@ describe('translations', () => {
103103
const stillAutotitle = texts.filter((text) => /autotitle/i.test(text))
104104
expect(stillAutotitle.length).toBe(0)
105105
})
106+
107+
test('markdown link looking constructs inside links', async () => {
108+
// On this page, the translators had written:
109+
//
110+
// [[Bar](バー)](/get-started/foo/bar)
111+
//
112+
// which needs to become:
113+
//
114+
// <a href="/ja/get-started/foo/bar">[Bar](バー)</a>
115+
const $ = await getDOM('/ja/get-started/start-your-journey/hello-world')
116+
const links = $('#article-contents a[href]')
117+
const texts = links
118+
.filter((i, element) => $(element).attr('href').includes('get-started/foo/bar'))
119+
.map((i, element) => $(element).text())
120+
.get()
121+
expect(texts.includes('[Bar] (バー)')).toBeTruthy()
122+
})
106123
})

src/languages/lib/correct-translation-content.js

Lines changed: 32 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -77,6 +77,38 @@ export function correctTranslatedContentStrings(content, englishContent, context
7777
'- % data variables.product.prodname_copilot_enterprise %}',
7878
'- {% data variables.product.prodname_copilot_enterprise %}',
7979
)
80+
81+
// This might not be exclusive to Japanese but put here because, at
82+
// the time of writing, it only happens on the Japanse translations.
83+
// According to the Microsoft translation guidelines, they're not
84+
// supposed to translate words that will be seen in the UI, but
85+
// instead mention then like this:
86+
//
87+
// [Save changes](THE TRANSLATION OF "Save changes" IN JAPANESE)
88+
//
89+
// The problem is when these are wrapped in a deliberate Markdown link.
90+
// For example:
91+
//
92+
// [[Save changes](THE TRANSLATION OF "Save changes" IN JAPANESE)](#some-section)
93+
//
94+
// A real observed example is:
95+
//
96+
// [[Allow deletions](削除を許可)](#allow-deletions)
97+
//
98+
// Here, because "削除を許可" contains no spaces, the Markdown parser
99+
// thinks "削除を許可" is the URL! But in actuality,
100+
// `[Allow deletions](削除を許可)` is the text and `#allow-deletions`
101+
// is the URL.
102+
// This problem does not exhibit if the text "削除を許可" were to contain
103+
// a space character. But we can't assume that we can just add a space.
104+
// For example "削除 を許可" would be incorrect. And where do you put the
105+
// space? Between which characters.
106+
// Instead, we can inject a "hair space" whitespace character between
107+
// the `]` and the `(`. Then, the Markdown processor does not get confused
108+
// and the link is rendered correctly.
109+
// The `\u200A` is the "hair space" character. Technically whitespace
110+
// but not wide enough to visually appear as a space.
111+
content = content.replace(/\[(\[.*?\])(\(\S+\)\]\()/g, '[$1\u200A$2')
80112
}
81113

82114
if (context.code === 'zh') {

0 commit comments

Comments
 (0)