diff --git a/.pnp.cjs b/.pnp.cjs index bcc548d..9736604 100644 --- a/.pnp.cjs +++ b/.pnp.cjs @@ -34,6 +34,7 @@ function $$SETUP_STATE(hydrateRuntimeState, basePath) { ["@octokit/rest", "npm:19.0.7"],\ ["@octokit/types", "npm:9.0.0"],\ ["@playwright/test", "npm:1.31.2"],\ + ["@primer/octicons-react", "virtual:ff5ad3439f8ec237c0c86796b437c422a681ce7f1211bc52c84c467fd5c19025673469e4b3bd047db74cf4144d670522e4013a081fcd63cc449dbcb3b5b92460#npm:18.2.0"],\ ["@primer/react", "virtual:ff5ad3439f8ec237c0c86796b437c422a681ce7f1211bc52c84c467fd5c19025673469e4b3bd047db74cf4144d670522e4013a081fcd63cc449dbcb3b5b92460#npm:35.20.0"],\ ["@react-three/drei", "virtual:ff5ad3439f8ec237c0c86796b437c422a681ce7f1211bc52c84c467fd5c19025673469e4b3bd047db74cf4144d670522e4013a081fcd63cc449dbcb3b5b92460#npm:9.57.1"],\ ["@react-three/fiber", "virtual:ff5ad3439f8ec237c0c86796b437c422a681ce7f1211bc52c84c467fd5c19025673469e4b3bd047db74cf4144d670522e4013a081fcd63cc449dbcb3b5b92460#npm:8.12.0"],\ @@ -4213,6 +4214,13 @@ function $$SETUP_STATE(hydrateRuntimeState, basePath) { ],\ "linkType": "SOFT"\ }],\ + ["npm:18.2.0", {\ + "packageLocation": "./.yarn/cache/@primer-octicons-react-npm-18.2.0-4922167f1d-95f1770f44.zip/node_modules/@primer/octicons-react/",\ + "packageDependencies": [\ + ["@primer/octicons-react", "npm:18.2.0"]\ + ],\ + "linkType": "SOFT"\ + }],\ ["virtual:02566b438c0e6066eb17807a9417eeeedfbeb730ff352bda25ea97131eff644f0590fd6ae4db6c892c79cc0e255483e826b0af8d4ec33f107675e1a30c98c4b8#npm:17.12.0", {\ "packageLocation": "./.yarn/__virtual__/@primer-octicons-react-virtual-d9b623413f/0/cache/@primer-octicons-react-npm-17.12.0-f2e14562a0-6af48465d2.zip/node_modules/@primer/octicons-react/",\ "packageDependencies": [\ @@ -4225,6 +4233,19 @@ function $$SETUP_STATE(hydrateRuntimeState, basePath) { "react"\ ],\ "linkType": "HARD"\ + }],\ + ["virtual:ff5ad3439f8ec237c0c86796b437c422a681ce7f1211bc52c84c467fd5c19025673469e4b3bd047db74cf4144d670522e4013a081fcd63cc449dbcb3b5b92460#npm:18.2.0", {\ + "packageLocation": "./.yarn/__virtual__/@primer-octicons-react-virtual-dd8ad50232/0/cache/@primer-octicons-react-npm-18.2.0-4922167f1d-95f1770f44.zip/node_modules/@primer/octicons-react/",\ + "packageDependencies": [\ + ["@primer/octicons-react", "virtual:ff5ad3439f8ec237c0c86796b437c422a681ce7f1211bc52c84c467fd5c19025673469e4b3bd047db74cf4144d670522e4013a081fcd63cc449dbcb3b5b92460#npm:18.2.0"],\ + ["@types/react", "npm:18.0.28"],\ + ["react", "npm:18.2.0"]\ + ],\ + "packagePeers": [\ + "@types/react",\ + "react"\ + ],\ + "linkType": "HARD"\ }]\ ]],\ ["@primer/primitives", [\ @@ -8903,6 +8924,7 @@ function $$SETUP_STATE(hydrateRuntimeState, basePath) { ["@octokit/rest", "npm:19.0.7"],\ ["@octokit/types", "npm:9.0.0"],\ ["@playwright/test", "npm:1.31.2"],\ + ["@primer/octicons-react", "virtual:ff5ad3439f8ec237c0c86796b437c422a681ce7f1211bc52c84c467fd5c19025673469e4b3bd047db74cf4144d670522e4013a081fcd63cc449dbcb3b5b92460#npm:18.2.0"],\ ["@primer/react", "virtual:ff5ad3439f8ec237c0c86796b437c422a681ce7f1211bc52c84c467fd5c19025673469e4b3bd047db74cf4144d670522e4013a081fcd63cc449dbcb3b5b92460#npm:35.20.0"],\ ["@react-three/drei", "virtual:ff5ad3439f8ec237c0c86796b437c422a681ce7f1211bc52c84c467fd5c19025673469e4b3bd047db74cf4144d670522e4013a081fcd63cc449dbcb3b5b92460#npm:9.57.1"],\ ["@react-three/fiber", "virtual:ff5ad3439f8ec237c0c86796b437c422a681ce7f1211bc52c84c467fd5c19025673469e4b3bd047db74cf4144d670522e4013a081fcd63cc449dbcb3b5b92460#npm:8.12.0"],\ diff --git a/package.json b/package.json index 6f9a53e..3f1215a 100644 --- a/package.json +++ b/package.json @@ -8,6 +8,7 @@ "@octokit/openapi-types": "^16.0.0", "@octokit/rest": "^19.0.7", "@octokit/types": "^9.0.0", + "@primer/octicons-react": "^18.2.0", "@primer/react": "^35.20.0", "@react-three/drei": "^9.57.1", "@react-three/fiber": "^8.11.1", diff --git a/src/chrome/web.ts b/src/chrome/web.ts index ed7e7dd..81c9b62 100644 --- a/src/chrome/web.ts +++ b/src/chrome/web.ts @@ -71,7 +71,7 @@ export function mapInjectableDiffElements( if (supportedElements.length !== supportedFiles.length) { throw Error( - `elements and files have different length. Got ${supportedElements.length} and ${files.length}` + `elements and files have different length. Got ${supportedElements.length} and ${supportedFiles.length}` ) } diff --git a/src/components/diff/CadDiff.test.tsx b/src/components/diff/CadDiff.test.tsx deleted file mode 100644 index 8844116..0000000 --- a/src/components/diff/CadDiff.test.tsx +++ /dev/null @@ -1,6 +0,0 @@ -import { render } from '@testing-library/react' -import { CadDiff } from './CadDiff' - -it('renders the CAD diff element', () => { - render() -}) diff --git a/src/components/diff/CadDiffPage.tsx b/src/components/diff/CadDiffPage.tsx index ed6caa2..2ba2540 100644 --- a/src/components/diff/CadDiffPage.tsx +++ b/src/components/diff/CadDiffPage.tsx @@ -1,10 +1,11 @@ import React, { useEffect, useState } from 'react' import '@react-three/fiber' -import { ThemeProvider } from '@primer/react' +import { Box, ThemeProvider } from '@primer/react' import { DiffEntry, FileDiff, MessageIds } from '../../chrome/types' import { createPortal } from 'react-dom' import { Loading } from '../Loading' import { CadDiff } from './CadDiff' +import { SourceRichToggle } from './SourceRichToggle' function CadDiffPortal({ element, @@ -21,17 +22,28 @@ function CadDiffPortal({ sha: string parentSha: string }): React.ReactElement { - const [diff, setDiff] = useState() - const [diffElement, setDiffElement] = useState() + const [richDiff, setRichDiff] = useState() + const [richSelected, setRichSelected] = useState(true) + const [toolbarContainer, setToolbarContainer] = useState() + const [diffContainer, setDiffContainer] = useState() + const [sourceElements, setSourceElements] = useState([]) + useEffect(() => { - const diffElement = element.querySelector( - '.js-file-content' - ) as HTMLElement - setDiffElement(diffElement) - // TODO: don't clean up once the rich/source toggle is added - for (const e of diffElement.childNodes) { - e.remove() + const toolbar = element.querySelector('.file-info') + if (toolbar != null) { + setToolbarContainer(toolbar) } + + const diff = element.querySelector('.js-file-content') + if (diff != null) { + setDiffContainer(diff) + const sourceElements = Array.from(diff.children) as HTMLElement[] + sourceElements.map(n => (n.style.display = 'none')) + setSourceElements(sourceElements) + } + }, [element]) + + useEffect(() => { ;(async () => { const response = await chrome.runtime.sendMessage({ id: MessageIds.GetFileDiff, @@ -40,20 +52,42 @@ function CadDiffPortal({ if ('error' in response) { console.log(response.error) } else { - setDiff(response as FileDiff) + setRichDiff(response as FileDiff) } })() - }, [element, diffElement, file, owner, repo, sha, parentSha]) + }, [file, owner, repo, sha, parentSha]) + return ( <> - {diffElement && + {toolbarContainer && createPortal( - diff ? ( - - ) : ( - - ), - diffElement + { + sourceElements.map(n => (n.style.display = 'block')) + setRichSelected(false) + }} + onRichSelected={() => { + sourceElements.map(n => (n.style.display = 'none')) + setRichSelected(true) + }} + />, + toolbarContainer + )} + {diffContainer && + createPortal( + + {richDiff ? ( + + ) : ( + + )} + , + diffContainer )} ) diff --git a/src/components/diff/SourceRichToggle.test.tsx b/src/components/diff/SourceRichToggle.test.tsx new file mode 100644 index 0000000..e2e77cb --- /dev/null +++ b/src/components/diff/SourceRichToggle.test.tsx @@ -0,0 +1,35 @@ +import { fireEvent, render, screen } from '@testing-library/react' +import { SourceRichToggle } from './SourceRichToggle' + +it('renders a source-rich diff toggle and checks its callbacks', async () => { + const callbackSource = jest.fn() + const callbackRich = jest.fn() + + render( + + ) + + const [sourceButton, richButton] = await screen.findAllByRole('button') + + expect(callbackSource.mock.calls).toHaveLength(0) + expect(callbackRich.mock.calls).toHaveLength(0) + + fireEvent.click(sourceButton) + expect(callbackSource.mock.calls).toHaveLength(1) + + fireEvent.click(richButton) + expect(callbackRich.mock.calls).toHaveLength(1) +}) + +it('renders a disbaled source-rich diff toggle', async () => { + render() + + const [sourceButton, richButton] = await screen.findAllByRole('button') + expect(sourceButton).toBeDisabled() + expect(richButton).toBeDisabled() +}) diff --git a/src/components/diff/SourceRichToggle.tsx b/src/components/diff/SourceRichToggle.tsx new file mode 100644 index 0000000..700333e --- /dev/null +++ b/src/components/diff/SourceRichToggle.tsx @@ -0,0 +1,59 @@ +import { ButtonGroup, IconButton, Tooltip } from '@primer/react' +import { PackageIcon, CodeIcon } from '@primer/octicons-react' + +export type SourceRichToggleProps = { + disabled: boolean + richSelected: boolean + onSourceSelected?: () => void + onRichSelected?: () => void +} + +export function SourceRichToggle({ + disabled, + richSelected, + onSourceSelected, + onRichSelected, +}: SourceRichToggleProps) { + const commonButtonSx = { + color: 'fg.subtle', + width: '40px', + } + const commonTooltipSx = { + height: '32px', + } + const sourceText = 'Display the source diff' + const richText = 'Display the rich diff' + return ( + + + + + + + + + ) +} diff --git a/tests/extension.spec.ts b/tests/extension.spec.ts index 3afc5d4..9fe897c 100644 --- a/tests/extension.spec.ts +++ b/tests/extension.spec.ts @@ -1,3 +1,4 @@ +import { Page } from '@playwright/test' import { test, expect } from './fixtures' test('popup page', async ({ page, extensionId }) => { @@ -17,16 +18,29 @@ test('authorized popup page', async ({ await expect(page.locator('button')).toHaveCount(2) }) +async function getFirstDiffScreenshot(page: Page, url: string) { + page.on('console', msg => console.log(msg.text())) + await page.goto(url) + + // waiting for the canvas (that holds the diff) to show up + await page.waitForSelector( + '.js-file[data-file-type=".obj"] .js-file-content canvas' + ) + + // screenshot the file diff with its toolbar + const element = await page.waitForSelector( + '.js-file[data-file-type=".obj"]' + ) + await page.waitForTimeout(1000) // making sure the element fully settled in + return await element.screenshot() +} + test('pull request diff with an .obj file', async ({ page, authorizedBackground, }) => { - page.on('console', msg => console.log(msg.text())) - - await page.goto('https://github.com/KittyCAD/kittycad.ts/pull/3/files') - const element = await page.waitForSelector('.js-file-content canvas') - await page.waitForTimeout(1000) // making sure the element fully settled in - const screenshot = await element.screenshot() + const url = 'https://github.com/KittyCAD/kittycad.ts/pull/3/files' + const screenshot = await getFirstDiffScreenshot(page, url) expect(screenshot).toMatchSnapshot() }) @@ -34,13 +48,8 @@ test('commit diff with an .obj file', async ({ page, authorizedBackground, }) => { - page.on('console', msg => console.log(msg.text())) - - await page.goto( + const url = 'https://github.com/KittyCAD/kittycad.ts/commit/08b50ee5a23b3ae7dd7b19383f14bbd520079cc1' - ) - const element = await page.waitForSelector('.js-file-content canvas') - await page.waitForTimeout(1000) // making sure the element fully settled in - const screenshot = await element.screenshot() + const screenshot = await getFirstDiffScreenshot(page, url) expect(screenshot).toMatchSnapshot() }) diff --git a/tests/extension.spec.ts-snapshots/commit-diff-with-an-obj-file-1-chromium-darwin.png b/tests/extension.spec.ts-snapshots/commit-diff-with-an-obj-file-1-chromium-darwin.png index d03f67f..8f77d2d 100644 Binary files a/tests/extension.spec.ts-snapshots/commit-diff-with-an-obj-file-1-chromium-darwin.png and b/tests/extension.spec.ts-snapshots/commit-diff-with-an-obj-file-1-chromium-darwin.png differ diff --git a/tests/extension.spec.ts-snapshots/commit-diff-with-an-obj-file-1-chromium-linux.png b/tests/extension.spec.ts-snapshots/commit-diff-with-an-obj-file-1-chromium-linux.png index 09fafff..f401080 100644 Binary files a/tests/extension.spec.ts-snapshots/commit-diff-with-an-obj-file-1-chromium-linux.png and b/tests/extension.spec.ts-snapshots/commit-diff-with-an-obj-file-1-chromium-linux.png differ diff --git a/tests/extension.spec.ts-snapshots/pull-request-diff-with-an-obj-file-1-chromium-darwin.png b/tests/extension.spec.ts-snapshots/pull-request-diff-with-an-obj-file-1-chromium-darwin.png index 78cba82..88d0b05 100644 Binary files a/tests/extension.spec.ts-snapshots/pull-request-diff-with-an-obj-file-1-chromium-darwin.png and b/tests/extension.spec.ts-snapshots/pull-request-diff-with-an-obj-file-1-chromium-darwin.png differ diff --git a/tests/extension.spec.ts-snapshots/pull-request-diff-with-an-obj-file-1-chromium-linux.png b/tests/extension.spec.ts-snapshots/pull-request-diff-with-an-obj-file-1-chromium-linux.png index 3fa0edc..31d1298 100644 Binary files a/tests/extension.spec.ts-snapshots/pull-request-diff-with-an-obj-file-1-chromium-linux.png and b/tests/extension.spec.ts-snapshots/pull-request-diff-with-an-obj-file-1-chromium-linux.png differ diff --git a/yarn.lock b/yarn.lock index 1f4c3e2..ff4c393 100644 --- a/yarn.lock +++ b/yarn.lock @@ -2524,6 +2524,15 @@ __metadata: languageName: node linkType: hard +"@primer/octicons-react@npm:^18.2.0": + version: 18.2.0 + resolution: "@primer/octicons-react@npm:18.2.0" + peerDependencies: + react: ">=15" + checksum: 95f1770f44628b3e0fe93133a89c615a14c2353af6074cf172e95200dd553c4171db418ce858dbbd6b5699431a5e51007cce325207836e209f6fe3ead0ba3758 + languageName: node + linkType: hard + "@primer/primitives@npm:7.10.0": version: 7.10.0 resolution: "@primer/primitives@npm:7.10.0" @@ -6089,6 +6098,7 @@ __metadata: "@octokit/rest": ^19.0.7 "@octokit/types": ^9.0.0 "@playwright/test": ^1.31.2 + "@primer/octicons-react": ^18.2.0 "@primer/react": ^35.20.0 "@react-three/drei": ^9.57.1 "@react-three/fiber": ^8.11.1