Skip to content
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.

Sign up for GitHub

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

fix: Update viewFile toggle in commit detail page #3623

Merged
merged 9 commits into from
Jan 6, 2025
Merged

fix: Update viewFile toggle in commit detail page #3623

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
be2a3b9
fix: Update viewFile toggle in commit detail page
suejung-sentry Jan 3, 2025
f12cf4f
fix test
suejung-sentry Jan 3, 2025
f3cf51d
Merge remote-tracking branch 'origin/main' into sshin/2399
suejung-sentry Jan 3, 2025
1558e21
Merge remote-tracking branch 'origin/main' into sshin/2399
suejung-sentry Jan 3, 2025
bbdda7e
fix other tables
suejung-sentry Jan 4, 2025
12d7511
fix tests
suejung-sentry Jan 4, 2025
497eacd
fix tests
suejung-sentry Jan 4, 2025
a2cc578
refactor onHover check
suejung-sentry Jan 6, 2025
10578f5
Merge remote-tracking branch 'origin/main' into sshin/2399
suejung-sentry Jan 6, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 2 additions & 8 deletions ...ilPage/CommitCoverage/routes/FilesChangedTab/FilesChangedTable/FilesChangedTable.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -134,14 +134,8 @@ describe('FilesChangedTable', () => {
const { queryClient } = setup(mockData)
render(<FilesChangedTable />, { wrapper: wrapper(queryClient) })

const link = await screen.findByRole('link', {
name: 'src/index2.py',
})
expect(link).toBeInTheDocument()
expect(link).toHaveAttribute(
'href',
'/gh/vax/keyleth/commit/123/blob/src/index2.py'
)
const text = await screen.findByText('src/index2.py')
expect(text).toBeInTheDocument()
})

it('renders coverage', async () => {
Expand Down
119 changes: 52 additions & 67 deletions ...tDetailPage/CommitCoverage/routes/FilesChangedTab/FilesChangedTable/FilesChangedTable.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,6 @@ import { useLocation, useParams } from 'react-router-dom'

import { ImpactedFileType, useCommit } from 'services/commit'
import { OrderingDirection, OrderingParameter } from 'services/pull/usePull'
import A from 'ui/A'
import Icon from 'ui/Icon'
import Spinner from 'ui/Spinner'
import TotalsNumber from 'ui/TotalsNumber'
Expand Down Expand Up @@ -71,7 +70,7 @@ function getFilter(sorting: Array<{ id: string; desc: boolean }>) {
return undefined
}

function getColumns({ commitId }: { commitId: string }) {
function getColumns() {
return [
columnHelper.accessor('headName', {
id: 'name',
Expand All @@ -83,42 +82,14 @@ function getColumns({ commitId }: { commitId: string }) {
return (
<div className="flex flex-row items-center break-all">
{!isDeletedFile ? (
<span
data-action="clickable"
data-testid="file-diff-expand"
className={cs(
'inline-flex items-center gap-1 font-sans hover:underline focus:ring-2',
{
'text-ds-blue-default': row.getIsExpanded(),
}
)}
{...{
onClick: row.getToggleExpandedHandler(),
}}
>
<Icon
size="md"
name={row.getIsExpanded() ? 'chevronDown' : 'chevronRight'}
variant="solid"
/>
</span>
<Icon
size="md"
name={row.getIsExpanded() ? 'chevronDown' : 'chevronRight'}
variant="solid"
className="flex-none"
/>
) : null}
{isDeletedFile ? (
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

very nice that we can delete this conditional logic now

<>{headName}</>
) : (
/* @ts-expect-error - A hasn't been typed yet */
<A
to={{
pageName: 'commitFileDiff',
options: {
commit: commitId,
tree: headName,
},
}}
>
{headName}
</A>
)}
<span>{headName}</span>
{row.original?.isCriticalFile ? (
<span className="ml-2 h-fit flex-none rounded border border-ds-gray-tertiary p-1 text-xs text-ds-gray-senary">
Critical file
Expand Down Expand Up @@ -286,7 +257,7 @@ export default function FilesChangedTable() {
}, [commit?.compareWithParent])

const table = useReactTable({
columns: getColumns({ commitId: commitSha }),
columns: getColumns(),
data: filesChanged,
state: {
expanded,
Expand All @@ -297,7 +268,7 @@ export default function FilesChangedTable() {
getCoreRowModel: getCoreRowModel(),
getSortedRowModel: getSortedRowModel(),
getExpandedRowModel: getExpandedRowModel(),
getRowCanExpand: () => true,
getRowCanExpand: (row) => row.original?.headCoverage !== null, // deleted files are not expandable
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this check used elsewhere (outside of these tables I mean)? Jw how we know this is the item to key off of when making a determination of if the file is deleted or not

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh good question. I pulled that from an existing usage from this table, but I get what you mean, maybe we should have this be a more inherent property of the file in api than having the UI make that designation here adhoc. And something more obvious than headCoverage being null (which can in theory happen for more reasons, not just due to a deleted file). I'll have to think about that one but in the meantime may ship this work independently

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yep totally fair! I'll keep this in the back of my head too in case I stumble upon a more straight-forward abstraction of this info at some point

})

if (isLoading || commit?.state === 'pending') {
Expand All @@ -321,7 +292,7 @@ export default function FilesChangedTable() {
}

return (
<div className="filelistui" data-highlight-row="onHover">
<div className="filelistui">
<div>
{table.getHeaderGroups().map((headerGroup) => (
<div key={headerGroup.id} className="filelistui-thead">
Expand Down Expand Up @@ -364,33 +335,47 @@ export default function FilesChangedTable() {
})}
</div>
))}
{table.getRowModel().rows.map((row, i) => (
<Fragment key={i}>
<div className="filelistui-row">
{row.getVisibleCells().map((cell) => {
return (
<div
key={cell.id}
{...(isNumericValue(cell.column.id)
? {
'data-type': 'numeric',
}
: {})}
className={cs({
'w-6/12': cell.column.id === 'name',
'w-2/12 justify-end flex': cell.column.id !== 'name',
})}
>
{flexRender(cell.column.columnDef.cell, cell.getContext())}
</div>
)
})}
</div>
<div data-expanded={row.getIsExpanded()}>
{row.getIsExpanded() ? <RenderSubComponent row={row} /> : null}
</div>
</Fragment>
))}
{table.getRowModel().rows.map((row, i) => {
const isDeletedFile = row.original?.headCoverage === null
return (
<Fragment key={i}>
<div
className={cs('filelistui-row', {
'cursor-pointer': !isDeletedFile,
'cursor-default': isDeletedFile,
})}
data-testid="file-diff-expand"
onClick={() => !isDeletedFile && row.toggleExpanded()}
data-highlight-row={!isDeletedFile ? 'onHover' : undefined}
>
{row.getVisibleCells().map((cell) => {
return (
<div
key={cell.id}
{...(isNumericValue(cell.column.id)
? {
'data-type': 'numeric',
}
: {})}
className={cs({
'w-6/12': cell.column.id === 'name',
'w-2/12 justify-end flex': cell.column.id !== 'name',
})}
>
{flexRender(
cell.column.columnDef.cell,
cell.getContext()
)}
</div>
)
})}
</div>
<div data-expanded={row.getIsExpanded()}>
{row.getIsExpanded() ? <RenderSubComponent row={row} /> : null}
</div>
</Fragment>
)
})}
</div>
</div>
)
Expand Down
8 changes: 1 addition & 7 deletions ...mmitCoverage/routes/IndirectChangesTab/IndirectChangesTable/IndirectChangesTable.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -123,14 +123,8 @@ describe('IndirectChangesTable', () => {
const { queryClient } = setup(mockData)
render(<IndirectChangesTable />, { wrapper: wrapper(queryClient) })

const link = await screen.findByRole('link', {
name: 'src/index2.py',
})
const link = await screen.findByText('src/index2.py')
expect(link).toBeInTheDocument()
expect(link).toHaveAttribute(
'href',
'/gh/codecov/cool-repo/commit/123/blob/src/index2.py'
)
})

it('renders coverage', async () => {
Expand Down
115 changes: 54 additions & 61 deletions ...ge/CommitCoverage/routes/IndirectChangesTab/IndirectChangesTable/IndirectChangesTable.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,6 @@ import { useLocation, useParams } from 'react-router-dom'

import ToggleHeader from 'pages/CommitDetailPage/Header/ToggleHeader/ToggleHeader'
import { ImpactedFileType, useCommit } from 'services/commit/useCommit'
import A from 'ui/A'
import Icon from 'ui/Icon'
import Spinner from 'ui/Spinner'
import TotalsNumber from 'ui/TotalsNumber'
Expand All @@ -28,44 +27,26 @@ const columnHelper = createColumnHelper<ImpactedFileType>()

const isNumericValue = (value: string) => value === 'head' || value === 'change'

function getColumns({ commitid }: { commitid: string }) {
function getColumns() {
return [
columnHelper.accessor('headName', {
id: 'name',
header: 'Name',
cell: ({ getValue, row }) => {
const headName = getValue()
const isDeletedFile = row.original?.headCoverage === null

return (
<div
className="flex cursor-pointer items-center gap-2"
data-testid="file-diff-expand"
onClick={() => row.toggleExpanded()}
>
<span
className={cs({
'text-ds-blue-darker': row.getIsExpanded(),
'text-current': !row.getIsExpanded(),
})}
>
<div className="flex items-center gap-2">
{!isDeletedFile ? (
<Icon
size="md"
name={row.getIsExpanded() ? 'chevronDown' : 'chevronRight'}
variant="solid"
className="flex-none"
/>
</span>
<div className="flex flex-col break-all">
<A
hook="commit-file-diff"
isExternal={false}
to={{
pageName: 'commitFileDiff',
options: { commit: commitid, tree: headName },
}}
>
{headName}
</A>
</div>
) : null}
<span>{headName}</span>
{row.original?.isCriticalFile ? (
<span className="flex-none self-center rounded border border-ds-gray-tertiary p-1 text-xs text-ds-gray-senary">
Critical file
Expand Down Expand Up @@ -183,14 +164,15 @@ export default function FilesChangedTable() {
}, [commitData?.commit?.compareWithParent])

const table = useReactTable({
columns: getColumns({ commitid: commitSha }),
columns: getColumns(),
data,
state: {
expanded,
},
onExpandedChange: setExpanded,
getCoreRowModel: getCoreRowModel(),
getExpandedRowModel: getExpandedRowModel(),
getRowCanExpand: (row) => row.original?.headCoverage !== null, // deleted files are not expandable
})

if (commitData?.commit?.state === 'pending') {
Expand Down Expand Up @@ -223,7 +205,7 @@ export default function FilesChangedTable() {
return (
<>
<ToggleHeader />
<div className="filelistui" data-highlight-row="onHover">
<div className="filelistui">
<div>
{table.getHeaderGroups().map((headerGroup) => (
<div key={headerGroup.id} className="filelistui-thead">
Expand All @@ -250,39 +232,50 @@ export default function FilesChangedTable() {
{isLoading ? (
<Loader />
) : (
table.getRowModel().rows.map((row, i) => (
<Fragment key={i}>
<div className="filelistui-row">
{row.getVisibleCells().map((cell) => {
return (
<div
key={cell.id}
{...(isNumericValue(cell.column.id)
? {
'data-type': 'numeric',
}
: {})}
className={cs({
'w-8/12': cell.column.id === 'name',
'flex justify-end': cell.column.id !== 'name',
'w-3/12': cell.column.id === 'change',
})}
>
{flexRender(
cell.column.columnDef.cell,
cell.getContext()
)}
</div>
)
})}
</div>
<div data-expanded={row.getIsExpanded()}>
{row.getIsExpanded() ? (
<RenderSubComponent row={row} />
) : null}
</div>
</Fragment>
))
table.getRowModel().rows.map((row, i) => {
const isDeletedFile = row.original?.headCoverage === null
return (
<Fragment key={i}>
<div
className={cs('filelistui-row', {
'cursor-pointer': !isDeletedFile,
'cursor-default': isDeletedFile,
})}
data-testid="file-diff-expand"
onClick={() => !isDeletedFile && row.toggleExpanded()}
data-highlight-row={!isDeletedFile ? 'onHover' : undefined}
>
{row.getVisibleCells().map((cell) => {
return (
<div
key={cell.id}
{...(isNumericValue(cell.column.id)
? {
'data-type': 'numeric',
}
: {})}
className={cs({
'w-8/12': cell.column.id === 'name',
'flex justify-end': cell.column.id !== 'name',
'w-3/12': cell.column.id === 'change',
})}
>
{flexRender(
cell.column.columnDef.cell,
cell.getContext()
)}
</div>
)
})}
</div>
<div data-expanded={row.getIsExpanded()}>
{row.getIsExpanded() ? (
<RenderSubComponent row={row} />
) : null}
</div>
</Fragment>
)
})
)}
</div>
</div>
Expand Down
2 changes: 1 addition & 1 deletion ...Coverage/routes/FilesChangedTab/FilesChanged/FilesChangedTable/FilesChangedTable.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -254,7 +254,7 @@ describe('FilesChangedTable', () => {
const { queryClient } = setup()
render(<FilesChangedTable />, { wrapper: wrapper(queryClient) })

const path = await screen.findByRole('link', { name: 'flag1/mafs.js' })
const path = await screen.findByText('flag1/mafs.js')
expect(path).toBeInTheDocument()
})

Expand Down
Loading
Loading