Skip to content

Commit

Permalink
Fix various issues on the Dashboard (#10256)
Browse files Browse the repository at this point in the history
- Fix React DevTools not working in Firefox
- Fix selection of asset names when editing them, not working at all in Firefox
- Convert tick/cross buttons when editing assets, and the "plus" and "reload" buttons on the "shared with" column, "labels" column, and keyboard shortcuts table, to match more with the rest of the design
- Update clip path when the container resizes, so that the icons for hidden columns never overlap the actual table header
- Fix #10184
- Fix renames being committed even when cancelling
- Fix duplicate name detection - previously, all asset types only checked folders with the same name, not assets with the same name
- I'm not 100% sure this is the correct behavior still
- Stop using `kbd` (`aria.Keyboard`) to display keyboard shortcuts, since they should not be displayed in a monospace font.
- Fix "plus" and "reload" buttons going past the right side of their parent table cell
- Limit length of `PermissionDisplay` - if the username of a user with permission is too long, it uses a tooltip instead
- Update the username dynamically for all permissions owned by self, when changing username in the settings.
- This avoids having to fully invalidate the directory tree every time the username changes, given that nothing changes about the assets' metadata themselves.
- Cache children in the Drive tree
- This avoids loading spinners when closing a folder and immediately reopening it.
- Note that children are still re-fetched on reopen to ensure freshness

# Important Notes
- This MAY be split into multiple smaller PRs. However, I think it's better to QA as a single PR, to avoid duplicating work checking behavior that may be changed by a sibling PR (assuming the PR was split into multiple).
  • Loading branch information
somebody1234 authored Jun 20, 2024
1 parent 83ec24d commit b5641aa
Show file tree
Hide file tree
Showing 78 changed files with 1,215 additions and 987 deletions.
37 changes: 15 additions & 22 deletions app/ide-desktop/eslint.config.js
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ import tsEslintParser from '@typescript-eslint/parser'
// === Constants ===
// =================

const DEBUG_STATEMENTS_MESSAGE = 'Avoid leaving debugging statements when committing code'
const DIR_NAME = path.dirname(url.fileURLToPath(import.meta.url))
const NAME = 'enso'
/** An explicit whitelist of CommonJS modules, which do not support namespace imports.
Expand Down Expand Up @@ -204,7 +205,7 @@ const RESTRICTED_SYNTAXES = [
{
selector: `JSXAttribute[name.name=/^(?:className)$/] TemplateLiteral`,
message:
'Use `tv` from `tailwind-variants` or `twMerge` from `tailwind-merge` instead of template strings for classes',
'Use `tv` from `#/utilities/tailwindVariants` or `twMerge` from `tailwind-merge` instead of template strings for classes',
},
{
selector: 'JSXOpeningElement[name.name=button] > JSXIdentifier',
Expand Down Expand Up @@ -283,11 +284,18 @@ export default [
'no-constant-condition': ['error', { checkLoops: false }],
'no-restricted-syntax': ['error', ...RESTRICTED_SYNTAXES],
'prefer-const': 'error',
'react/forbid-elements': [
'error',
{ forbid: [{ element: 'Debug', message: DEBUG_STATEMENTS_MESSAGE }] },
],
// Not relevant because TypeScript checks types.
'react/prop-types': 'off',
'react/self-closing-comp': 'error',
'react-hooks/rules-of-hooks': 'error',
'react-hooks/exhaustive-deps': 'error',
'react-hooks/exhaustive-deps': [
'error',
{ additionalHooks: 'useOnScroll|useStickyTableHeaderOnScroll' },
],
'react/jsx-pascal-case': ['error', { allowNamespace: true }],
// Prefer `interface` over `type`.
'@typescript-eslint/consistent-type-definitions': 'error',
Expand Down Expand Up @@ -473,26 +481,11 @@ export default [
property: 'useNavigate',
message: 'Use `hooks.useNavigate` instead.',
},
{
object: 'console',
message: 'Avoid leaving debugging statements when committing code',
},
{
property: 'useDebugState',
message: 'Avoid leaving debugging statements when committing code',
},
{
property: 'useDebugEffect',
message: 'Avoid leaving debugging statements when committing code',
},
{
property: 'useDebugMemo',
message: 'Avoid leaving debugging statements when committing code',
},
{
property: 'useDebugCallback',
message: 'Avoid leaving debugging statements when committing code',
},
{ object: 'console', message: DEBUG_STATEMENTS_MESSAGE },
{ property: 'useDebugState', message: DEBUG_STATEMENTS_MESSAGE },
{ property: 'useDebugEffect', message: DEBUG_STATEMENTS_MESSAGE },
{ property: 'useDebugMemo', message: DEBUG_STATEMENTS_MESSAGE },
{ property: 'useDebugCallback', message: DEBUG_STATEMENTS_MESSAGE },
],
},
},
Expand Down
5 changes: 2 additions & 3 deletions app/ide-desktop/lib/assets/cross.svg
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
9 changes: 3 additions & 6 deletions app/ide-desktop/lib/assets/plus2.svg
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
8 changes: 8 additions & 0 deletions app/ide-desktop/lib/assets/reload.svg
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
20 changes: 0 additions & 20 deletions app/ide-desktop/lib/assets/reload_in_circle.svg

This file was deleted.

4 changes: 2 additions & 2 deletions app/ide-desktop/lib/assets/tick.svg
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
7 changes: 7 additions & 0 deletions app/ide-desktop/lib/dashboard/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,13 @@
The dashboard is the entrypoint into the application. It includes project
management, project sharing, and user accounts and authentication.

## Further documentation

Further documentation is provided in the `docs/` folder:

- [Browser-specific behavior](./docs/browser_specific_behavior.md) details
behavior that is inconsistent between browsers and needs to be worked around.

## Folder structure

- `mock/`: Overrides for specific files in `src/` when running Playwright tests.
Expand Down
65 changes: 65 additions & 0 deletions app/ide-desktop/lib/dashboard/docs/browser_specific_behavior.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,65 @@
# Browser-specific behavior

This document details behavior that is inconsistent between browsers and needs
to be worked around.

## List of inconsistent behaviors

### Drag event missing coordinates

Firefox sets `MouseEvent.pageX` and `MouseEvent.pageY` to `0` for `drag`
events.

#### Fix

Pass the `drag` event handlers to `dragover` event as well, and wrap all `drag`
event handlers in:

````ts
if (event.pageX !== 0 || event.pageY !== 0) {
// original body here
}
```

#### Affected files

- [`DragModal.tsx`](../src/modals/DragModal.tsx)

### Drag event propagation in text inputs

Text selection in text inputs DO NOT WORK on Firefox, when the text input is a
child of an element with `draggable="true"`.
See [Firefox bug 800050].
To solve this problem, use `useDraggable` from
[`dragAndDropHooks.ts`] on ALL elements that MAY contain a text input.

[Firefox bug 800050]: https://bugzilla.mozilla.org/show_bug.cgi?id=800050

#### Fix

Merge `useDraggable` from [`dragAndDropHooks.ts`] on ALL elements that MAY
contain a text input.

It is recommended to use `aria.mergeProps` to combine these props with existing
props.

```tsx
import * as dragAndDropHooks from "#/hooks/dragAndDropHooks.ts";
const draggableProps = dragAndDropHooks.useDraggable();
return <div {...draggableProps}></div>;
````
[`draggableHooks.ts`]: ../src/hooks/dragAndDropHooks.ts
#### Affected browsers
- Firefox (all versions)
#### Affected files
- [`EditableSpan.tsx`](../src/components/EditableSpan.tsx) - the text inputs
that are affected
- [`AssetRow.tsx`](../src/components/dashboard/AssetRow.tsx) - fixes text
selection in `EditableSpan.tsx`
4 changes: 2 additions & 2 deletions app/ide-desktop/lib/dashboard/e2e/actions.ts
Original file line number Diff line number Diff line change
Expand Up @@ -142,12 +142,12 @@ export function locateLabelsPanelLabels(page: test.Page) {

/** Find a tick button (if any) on the current page. */
export function locateEditingTick(page: test.Locator | test.Page) {
return page.getByAltText('Confirm Edit')
return page.getByLabel('Confirm Edit')
}

/** Find a cross button (if any) on the current page. */
export function locateEditingCross(page: test.Locator | test.Page) {
return page.getByAltText('Cancel Edit')
return page.getByLabel('Cancel Edit')
}

/** Find labels in the "Labels" column of the assets table (if any) on the current page. */
Expand Down
3 changes: 2 additions & 1 deletion app/ide-desktop/lib/dashboard/e2e/assetPanel.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,8 @@ test.test('asset panel contents', ({ page }) =>
permission: permissions.PermissionAction.own,
user: {
organizationId: defaultOrganizationId,
userId: defaultUserId,
// Using the default ID causes the asset to have a dynamic username.
userId: backend.UserId(defaultUserId + '2'),
name: USERNAME,
email: backend.EmailAddress(EMAIL),
},
Expand Down
75 changes: 35 additions & 40 deletions app/ide-desktop/lib/dashboard/e2e/editAssetName.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,85 +8,80 @@ test.test.beforeEach(actions.mockAllAndLogin)
test.test('edit name', async ({ page }) => {
const assetRows = actions.locateAssetRows(page)
const mod = await actions.modModifier(page)
const row = assetRows.nth(0)
const newName = 'foo bar baz'

await actions.locateNewFolderIcon(page).click()
await actions.locateAssetRowName(assetRows.nth(0)).click({ modifiers: [mod] })
await actions.locateAssetRowName(assetRows.nth(0)).fill(newName)
await actions.locateEditingTick(assetRows.nth(0)).click()
await test.expect(assetRows).toHaveCount(1)
await test.expect(assetRows.nth(0)).toBeVisible()
await test.expect(assetRows.nth(0)).toHaveText(new RegExp('^' + newName))
await actions.locateAssetRowName(row).click({ modifiers: [mod] })
await actions.locateAssetRowName(row).fill(newName)
await actions.locateEditingTick(row).click()
await test.expect(row).toHaveText(new RegExp('^' + newName))
})

test.test('edit name (keyboard)', async ({ page }) => {
const assetRows = actions.locateAssetRows(page)
const row = assetRows.nth(0)
const newName = 'foo bar baz quux'

await actions.locateNewFolderIcon(page).click()
await actions.locateAssetRowName(assetRows.nth(0)).click()
await actions.locateAssetRowName(row).click()
await actions.press(page, 'Mod+R')
await actions.locateAssetRowName(assetRows.nth(0)).fill(newName)
await actions.locateAssetRowName(assetRows.nth(0)).press('Enter')
await test.expect(assetRows).toHaveCount(1)
await test.expect(assetRows.nth(0)).toBeVisible()
await test.expect(assetRows.nth(0)).toHaveText(new RegExp('^' + newName))
await actions.locateAssetRowName(row).fill(newName)
await actions.locateAssetRowName(row).press('Enter')
await test.expect(row).toHaveText(new RegExp('^' + newName))
})

test.test('cancel editing name', async ({ page }) => {
const assetRows = actions.locateAssetRows(page)
const mod = await actions.modModifier(page)
const row = assetRows.nth(0)
const newName = 'foo bar baz'

await actions.locateNewFolderIcon(page).click()
const oldName = (await actions.locateAssetRowName(assetRows.nth(0)).textContent()) ?? ''
await actions.locateAssetRowName(assetRows.nth(0)).click({ modifiers: [mod] })
await actions.locateAssetRowName(assetRows.nth(0)).fill(newName)
await actions.locateEditingCross(assetRows.nth(0)).click()
await test.expect(assetRows).toHaveCount(1)
await test.expect(assetRows.nth(0)).toBeVisible()
await test.expect(assetRows.nth(0)).toHaveText(new RegExp('^' + oldName))
const oldName = (await actions.locateAssetRowName(row).textContent()) ?? ''
await actions.locateAssetRowName(row).click({ modifiers: [mod] })
await actions.locateAssetRowName(row).fill(newName)
await actions.locateEditingCross(row).click()
await test.expect(row).toHaveText(new RegExp('^' + oldName))
})

test.test('cancel editing name (keyboard)', async ({ page }) => {
const assetRows = actions.locateAssetRows(page)
const row = assetRows.nth(0)
const newName = 'foo bar baz quux'

await actions.locateNewFolderIcon(page).click()
const oldName = (await actions.locateAssetRowName(assetRows.nth(0)).textContent()) ?? ''
await actions.locateAssetRowName(assetRows.nth(0)).click()
const oldName = (await actions.locateAssetRowName(row).textContent()) ?? ''
await actions.locateAssetRowName(row).click()
await actions.press(page, 'Mod+R')
await actions.locateAssetRowName(assetRows.nth(0)).fill(newName)
await actions.locateAssetRowName(assetRows.nth(0)).press('Escape')
await test.expect(assetRows).toHaveCount(1)
await test.expect(assetRows.nth(0)).toBeVisible()
await test.expect(assetRows.nth(0)).toHaveText(new RegExp('^' + oldName))
await actions.locateAssetRowName(row).fill(newName)
await actions.locateAssetRowName(row).press('Escape')
await test.expect(row).toHaveText(new RegExp('^' + oldName))
})

test.test('change to blank name', async ({ page }) => {
const assetRows = actions.locateAssetRows(page)
const mod = await actions.modModifier(page)
const row = assetRows.nth(0)

await actions.locateNewFolderIcon(page).click()
const oldName = (await actions.locateAssetRowName(assetRows.nth(0)).textContent()) ?? ''
await actions.locateAssetRowName(assetRows.nth(0)).click({ modifiers: [mod] })
await actions.locateAssetRowName(assetRows.nth(0)).fill('')
await actions.locateEditingTick(assetRows.nth(0)).click()
await test.expect(assetRows).toHaveCount(1)
await test.expect(assetRows.nth(0)).toBeVisible()
await test.expect(assetRows.nth(0)).toHaveText(new RegExp('^' + oldName))
const oldName = (await actions.locateAssetRowName(row).textContent()) ?? ''
await actions.locateAssetRowName(row).click({ modifiers: [mod] })
await actions.locateAssetRowName(row).fill('')
await test.expect(actions.locateEditingTick(row)).not.toBeVisible()
await actions.locateEditingCross(row).click()
await test.expect(row).toHaveText(new RegExp('^' + oldName))
})

test.test('change to blank name (keyboard)', async ({ page }) => {
const assetRows = actions.locateAssetRows(page)
const row = assetRows.nth(0)

await actions.locateNewFolderIcon(page).click()
const oldName = (await actions.locateAssetRowName(assetRows.nth(0)).textContent()) ?? ''
await actions.locateAssetRowName(assetRows.nth(0)).click()
const oldName = (await actions.locateAssetRowName(row).textContent()) ?? ''
await actions.locateAssetRowName(row).click()
await actions.press(page, 'Mod+R')
await actions.locateAssetRowName(assetRows.nth(0)).fill('')
await actions.locateAssetRowName(assetRows.nth(0)).press('Enter')
await test.expect(assetRows).toHaveCount(1)
await test.expect(assetRows.nth(0)).toBeVisible()
await test.expect(assetRows.nth(0)).toHaveText(new RegExp('^' + oldName))
await actions.locateAssetRowName(row).fill('')
await actions.locateAssetRowName(row).press('Enter')
await test.expect(row).toHaveText(new RegExp('^' + oldName))
})
Loading

0 comments on commit b5641aa

Please sign in to comment.