-
Notifications
You must be signed in to change notification settings - Fork 3.7k
improvement(files): match CSV/XLSX preview tables to the markdown table chrome #6125
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merged
Merged
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
46 changes: 46 additions & 0 deletions
46
apps/sim/app/workspace/[workspaceId]/files/components/file-viewer/document-table.css
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,46 @@ | ||
| /* | ||
| * Canonical table chrome for the file viewer. Both surfaces that render a table for a file — the | ||
| * rich markdown editor (`.rich-markdown-prose table`) and the tabular previews CSV/XLSX render | ||
| * through `DataTable` (`.document-table`) — share this one definition so a table looks the same | ||
| * whichever file it came from. Editor-only concerns (prose block margin, fixed layout for column | ||
| * resizing, cell paragraph reset) stay in rich-markdown-editor.css. | ||
| */ | ||
|
|
||
| /* `overflow-wrap` matches what `.rich-markdown-prose` sets on its own root: cells hold arbitrary | ||
| file data, so an unbreakable token (a URL, a hash) must break rather than overflow its cell. */ | ||
| .document-table { | ||
| color: var(--text-primary); | ||
| overflow-wrap: anywhere; | ||
| } | ||
|
|
||
| .rich-markdown-prose table, | ||
| .document-table table { | ||
| width: 100%; | ||
| border-collapse: collapse; | ||
| overflow: hidden; | ||
| } | ||
|
|
||
| .rich-markdown-prose th, | ||
| .rich-markdown-prose td, | ||
| .document-table th, | ||
| .document-table td { | ||
| position: relative; | ||
| border: 1px solid var(--divider); | ||
| padding: 0.5rem 0.75rem; | ||
| text-align: left; | ||
| vertical-align: top; | ||
| font-size: 14px; | ||
| line-height: 1.5rem; | ||
| } | ||
|
|
||
| .rich-markdown-prose th, | ||
| .document-table th { | ||
| background: var(--surface-4); | ||
| font-weight: 600; | ||
| } | ||
|
|
||
| /* Cell editors are invisible until focused: they take the cell's own typography and color. */ | ||
| .document-table input { | ||
| font: inherit; | ||
| color: inherit; | ||
| } | ||
155 changes: 155 additions & 0 deletions
155
apps/sim/app/workspace/[workspaceId]/files/components/file-viewer/document-table.test.ts
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,155 @@ | ||
| /** | ||
| * @vitest-environment jsdom | ||
| * | ||
| * A table must look the same whichever file it came from: a markdown file rendered by the rich | ||
| * markdown editor (`.rich-markdown-prose table`) and a CSV/XLSX preview rendered by `DataTable` | ||
| * (`.document-table`) sit in the same file viewer, in the same session, one click apart. They used | ||
| * to drift — the previews carried their own chrome (rounded outer frame, `--surface-2` header, | ||
| * 13px body / 12px header, `--text-secondary` cells) while markdown tables used full cell borders | ||
| * on `--divider`, a `--surface-4` header, and 14px text. | ||
| * | ||
| * These load the real, shipped CSS (not a copy). Two complementary assertions, because jsdom's CSS | ||
| * engine resolves only part of what matters here: it applies the cascade for longhand declarations | ||
| * (so `getComputedStyle` parity is a real check on padding/typography/fill), but it does not expand | ||
| * the `border` shorthand at all. Borders are therefore checked structurally — one rule, whose | ||
| * selector list covers both roots — which is also the property that actually prevents drift. | ||
| */ | ||
| import { readFileSync } from 'node:fs' | ||
| import path from 'node:path' | ||
| import { afterEach, beforeAll, describe, expect, it } from 'vitest' | ||
|
|
||
| const SHARED_CSS_PATH = path.join(__dirname, 'document-table.css') | ||
| const EDITOR_CSS_PATH = path.join(__dirname, 'rich-markdown-editor', 'rich-markdown-editor.css') | ||
|
|
||
| /** Chrome both surfaces must agree on, as longhands jsdom resolves. */ | ||
| const SHARED_CELL_PROPS = [ | ||
| 'padding-top', | ||
| 'padding-right', | ||
| 'padding-bottom', | ||
| 'padding-left', | ||
| 'font-size', | ||
| 'line-height', | ||
| 'text-align', | ||
| 'vertical-align', | ||
| ] as const | ||
|
|
||
| const sheets = new Map<string, CSSStyleSheet>() | ||
|
|
||
| beforeAll(() => { | ||
| for (const file of [SHARED_CSS_PATH, EDITOR_CSS_PATH]) { | ||
| const style = document.createElement('style') | ||
| style.textContent = readFileSync(file, 'utf-8') | ||
| document.head.appendChild(style) | ||
| if (!style.sheet) throw new Error(`stylesheet did not parse: ${file}`) | ||
| sheets.set(file, style.sheet) | ||
| } | ||
| }) | ||
|
|
||
| let containers: HTMLDivElement[] = [] | ||
|
|
||
| afterEach(() => { | ||
| for (const c of containers) c.remove() | ||
| containers = [] | ||
| }) | ||
|
|
||
| /** Mounts a one-cell table inside `rootClass` and returns the root plus its `th` and `td`. */ | ||
| function mountTable(rootClass: string): { root: HTMLElement; th: HTMLElement; td: HTMLElement } { | ||
| const container = document.createElement('div') | ||
| container.className = rootClass | ||
| container.innerHTML = | ||
| '<table><thead><tr><th>h</th></tr></thead><tbody><tr><td>c</td></tr></tbody></table>' | ||
| document.body.appendChild(container) | ||
| containers.push(container) | ||
| const th = container.querySelector('th') | ||
| const td = container.querySelector('td') | ||
| if (!th || !td) throw new Error('table cells not found') | ||
| return { root: container, th, td } | ||
| } | ||
|
|
||
| function declarations(el: Element, props: readonly string[]): Record<string, string> { | ||
| const computed = getComputedStyle(el) | ||
| return Object.fromEntries(props.map((p) => [p, computed.getPropertyValue(p)])) | ||
| } | ||
|
|
||
| /** Style rules of one loaded stylesheet, in source order. */ | ||
| function styleRules(cssPath: string): CSSStyleRule[] { | ||
| const sheet = sheets.get(cssPath) | ||
| if (!sheet) throw new Error(`stylesheet not loaded: ${cssPath}`) | ||
| return Array.from(sheet.cssRules).filter((r): r is CSSStyleRule => r instanceof CSSStyleRule) | ||
| } | ||
|
|
||
| /** Selector list of the single shared rule declaring `property: value`, as trimmed selectors. */ | ||
| function selectorsDeclaring(cssPath: string, property: string, value: string): string[] { | ||
| const matching = styleRules(cssPath).filter((r) => | ||
| r.style.getPropertyValue(property).includes(value) | ||
| ) | ||
| expect(matching).toHaveLength(1) | ||
| return matching[0].selectorText.split(',').map((s) => s.trim()) | ||
| } | ||
|
|
||
| describe('document-table chrome is shared with markdown tables', () => { | ||
| it('cells resolve to identical padding and typography', () => { | ||
| const prose = mountTable('rich-markdown-prose') | ||
| const preview = mountTable('document-table') | ||
|
|
||
| expect(declarations(preview.td, SHARED_CELL_PROPS)).toEqual( | ||
| declarations(prose.td, SHARED_CELL_PROPS) | ||
| ) | ||
| expect(declarations(preview.th, SHARED_CELL_PROPS)).toEqual( | ||
| declarations(prose.th, SHARED_CELL_PROPS) | ||
| ) | ||
| }) | ||
|
|
||
| /** | ||
| * Cells hold arbitrary file data, so an unbreakable token (a URL, a hash) must break rather than | ||
| * overflow — the previews lost `whitespace-nowrap` and would otherwise have no wrapping rule at | ||
| * all. `overflow-wrap` is inherited from each surface's root (jsdom does not propagate inherited | ||
| * properties to descendants, so the roots are what can be asserted). | ||
| */ | ||
| it('both roots declare the same wrapping for unbreakable cell values', () => { | ||
| const prose = mountTable('rich-markdown-prose') | ||
| const preview = mountTable('document-table') | ||
|
|
||
| const wrap = getComputedStyle(prose.root).getPropertyValue('overflow-wrap') | ||
| expect(wrap).toBe('anywhere') | ||
| expect(getComputedStyle(preview.root).getPropertyValue('overflow-wrap')).toBe(wrap) | ||
| }) | ||
|
|
||
| it('the resolved values are the markdown editor values, not jsdom defaults', () => { | ||
| const { th, td } = mountTable('document-table') | ||
|
|
||
| expect(getComputedStyle(td).getPropertyValue('padding-left')).toBe('0.75rem') | ||
| expect(getComputedStyle(td).getPropertyValue('font-size')).toBe('14px') | ||
| expect(getComputedStyle(th).getPropertyValue('font-weight')).toBe('600') | ||
| }) | ||
|
|
||
| it('one rule draws the cell border for both roots', () => { | ||
| expect(selectorsDeclaring(SHARED_CSS_PATH, 'border', 'var(--divider)')).toEqual( | ||
| expect.arrayContaining([ | ||
| '.rich-markdown-prose th', | ||
| '.rich-markdown-prose td', | ||
| '.document-table th', | ||
| '.document-table td', | ||
| ]) | ||
| ) | ||
| }) | ||
|
|
||
| it('one rule fills the header for both roots', () => { | ||
| expect(selectorsDeclaring(SHARED_CSS_PATH, 'background', 'var(--surface-4)')).toEqual( | ||
| expect.arrayContaining(['.rich-markdown-prose th', '.document-table th']) | ||
| ) | ||
| }) | ||
|
|
||
| it('the editor stylesheet no longer re-declares cell chrome of its own', () => { | ||
| const redeclared = styleRules(EDITOR_CSS_PATH).filter( | ||
| (r) => | ||
| /\.rich-markdown-prose (th|td)\b/.test(r.selectorText) && | ||
| (r.style.getPropertyValue('border') || | ||
| r.style.getPropertyValue('padding') || | ||
| r.style.getPropertyValue('font-size') || | ||
| r.style.getPropertyValue('background')) | ||
| ) | ||
|
|
||
| expect(redeclared.map((r) => r.selectorText)).toEqual([]) | ||
| }) | ||
| }) |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading. Please reload this page.