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

Jump to bottom

Svelte: Implement and instantiate FilePopover #62498

Merged
merged 5 commits into from May 21, 2024
Merged

Svelte: Implement and instantiate FilePopover #62498

merged 5 commits into from May 21, 2024
+377 −17

Conversation

jasonhawkharris
Copy link
Contributor

@jasonhawkharris jasonhawkharris commented May 7, 2024 •
edited by camdencheek

This PR Implements and instantiates the file popover on the SvelteKit web app.

Dark Mode:
https://github.com/sourcegraph/sourcegraph/assets/62355966/f1405cc2-54e0-49ba-aa26-7ddf2d258970

Light Mode:
https://github.com/sourcegraph/sourcegraph/assets/62355966/a8eb53ed-2a38-4574-a76a-5f5559ded015

Questions:

  1. This is working very well, but is there a better way to implement it?

Test plan

Added a storybook test and a playwright test

@cla-bot cla-bot bot added the cla-signed label May 7, 2024
@jasonhawkharris jasonhawkharris force-pushed the jhh/file-popover branch 4 times, most recently from b95b8d0 to 73a02dd Compare May 13, 2024 14:19
@jasonhawkharris jasonhawkharris changed the title Jhh/file popover Svelte: Implement and instantiate FilePopover May 13, 2024
@jasonhawkharris jasonhawkharris marked this pull request as ready for review May 13, 2024 14:46
camdencheek
camdencheek reviewed May 13, 2024
View reviewed changes
.tool-versions Outdated Show resolved Hide resolved
camdencheek
camdencheek reviewed May 13, 2024
View reviewed changes
Copy link
Member

@camdencheek camdencheek left a comment

Choose a reason for hiding this comment

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

A couple of general comments about the design we should follow up on.

The top line is almost always going to get cut off because we're trying to fit a full repo name and a large path into a small space:

screenshot-2024-05-13_14-23-38@2x

Long commit summaries should be wrapped, not truncated. And we should linkify the PR number.

screenshot-2024-05-13_14-26-19@2x

The radius of the popover background does not match the border radius of its border.
screenshot-2024-05-13_14-27-28@2x

camdencheek
camdencheek reviewed May 13, 2024
View reviewed changes
client/web-sveltekit/src/lib/repo/filePopover/FilePopover.stories.svelte Outdated
Copy link
Member

Choose a reason for hiding this comment

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

Viewing this in storybook fails for me with can't access property "split", filePath2 is undefined

@jasonhawkharris
Copy link
Contributor Author

jasonhawkharris commented May 13, 2024 •
edited

Long commit summaries should be wrapped, not truncated. And we should linkify the PR number.

@camdencheek Do we actually need the repository name? I feel like the file path should suffice, since by the time the user gets to a file tree view, they'll more than likely know what repo they're in. Right?

camdencheek
camdencheek reviewed May 13, 2024
View reviewed changes
Copy link
Member

@camdencheek camdencheek left a comment

Choose a reason for hiding this comment

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

Looks a little odd that there is no border here

screenshot-2024-05-13_15-05-19@2x

@jasonhawkharris
Copy link
Contributor Author

Looks a little odd that there is no border here

This was part of the original design. I have to say, I like it better in dark mode. It does look a little awkward in light mode.

@jasonhawkharris jasonhawkharris force-pushed the jhh/file-popover branch 5 times, most recently from 1e6174b to d14df94 Compare May 14, 2024 18:08
camdencheek
camdencheek reviewed May 14, 2024
View reviewed changes
client/web-sveltekit/src/lib/Popover.svelte Outdated
border-radius: var(--popover-border-radius);
border-radius: var(--border-radius);
Copy link
Member

Choose a reason for hiding this comment

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

Why did this change? Other components may be depending on being able to set --popover-border-radius

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There are two values. --popover-border-radius (equal to 5px) and --border-radius (equal to 3px).

Copy link
Member

Choose a reason for hiding this comment

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

Right, why did you change the Popover component to use --border-radius?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ahhh... I see what you're saying. I'll change it.

camdencheek
camdencheek reviewed May 14, 2024
View reviewed changes
client/web-sveltekit/src/lib/repo/filePopover/FilePopover.svelte Outdated
Comment on lines 26 to 50
const fetchPopoverData = async () => {
const client = getGraphQLClient()
const result = await client.query(FileOrDirPopoverQuery, {
repoName,
revspec,
filePath,
})

if (result.error || result.data === undefined) {
console.error('could not fetch file or dir popover', result.error)
throw new Error('could not fetch file or dir popover', result.error)
}

if (result?.data?.repository?.commit?.path?.__typename === 'GitBlob') {
if (result?.data.repository?.commit?.path?.blame.length > 0) {
filePopover = result?.data?.repository?.commit?.path
}
return filePopover
} else if (result?.data?.repository?.commit?.path?.__typename === 'GitTree') {
dirPopover = result?.data?.repository?.commit?.path
return dirPopover
}

return null
}
Copy link
Member

Choose a reason for hiding this comment

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

Why did we depart from the pattern established for RepoPopover and do the data fetching in the component rather than in the +page.ts?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@camdencheek Long story short, I tried to implement it around 3 or 4 times and it was a giant pain each time, always with many bugs to work through. Why does it matter if we do it here? It seems to be better here because it's contained directly in the component that calls it. It also only mounts on hover, so we're only calling it when we need it.

Copy link
Member

Choose a reason for hiding this comment

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

In general, we want to separate data fetching from rendering because the source of the data might be different when we use the component in different places. e.g., we may want to batch these calls, or cancel a call if you hover over something different, or preload the data for some locations.

Another more concrete reason is that this is now much harder to test because, rather than just passing in the data it needs to render, you now need to mock out the API call.

Copy link
Member

Choose a reason for hiding this comment

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

Another concrete example is, if you re-hover, we now re-fetch because the component is re-created.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay. I see.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In that case, with the little time I have left, I think I need to cry "uncle" here and pass this along to someone else. I don't see me getting this done today. I've tried a number of times and failed to reproduce what we did with RepoPopover.

@taiyab
Copy link
Contributor

taiyab commented May 20, 2024

With @jasonhawkharris on leave, any chance this gets merged anytime soon? @camdencheek

@camdencheek
Copy link
Member

Yep, this is first on my list tomorrow

@camdencheek camdencheek self-assigned this May 21, 2024
@camdencheek camdencheek merged commit 939e000 into main May 21, 2024
10 checks passed
@camdencheek camdencheek deleted the jhh/file-popover branch May 21, 2024 19:26
@taiyab
Copy link
Contributor

taiyab commented May 22, 2024

@camdencheek @jasonhawkharris As a follow up to this:

  • The path needs each individual dir name to be linked
  • We need a solution to show the full path, always (so decently presented wrapping, slightly wider popover maybe too, condensing space between / and dir names a bit)
  • link the commit message to the commit too

@taiyab taiyab mentioned this pull request May 22, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@camdencheek camdencheek camdencheek approved these changes

@fkling fkling Awaiting requested review from fkling

Assignees

@camdencheek camdencheek

Labels
cla-signed
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@jasonhawkharris @taiyab @camdencheek