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.

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

feat(replays): Create a "URLWalker" breadcrumb component #37038

Merged
merged 8 commits into from
Jul 29, 2022

Conversation

ryan953
Copy link
Member

@ryan953 ryan953 commented Jul 26, 2022

Some permutations in the details page:
Screen Shot 2022-07-27 at 10 31 33 AM
Screen Shot 2022-07-27 at 10 31 41 AM
Screen Shot 2022-07-27 at 10 31 49 AM
Screen Shot 2022-07-27 at 10 53 21 AM
Screen Shot 2022-07-27 at 10 36 12 AM

And more on the index page:
Screen Shot 2022-07-27 at 10 33 50 AM
Screen Shot 2022-07-27 at 10 34 30 AM
Screen Shot 2022-07-27 at 10 48 52 AM
Screen Shot 2022-07-27 at 10 54 05 AM
Screen Shot 2022-07-27 at 10 52 38 AM

The big difference between the list page and details, is that on the list page we don't expect to get timestamps for each navigation event. So the relative time doesn't appear in the Hovercard.

@github-actions
Copy link
Contributor

github-actions bot commented Jul 27, 2022

size-limit report 📦

Path Size
src/sentry/static/sentry/dist/entrypoints/app.js 63.13 KB (+0.16% 🔺)
src/sentry/static/sentry/dist/entrypoints/sentry.css 70.87 KB (0%)

Copy link
Member

@billyvg billyvg left a comment

Choose a reason for hiding this comment

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

I don't love the -Walker name - to me, it sounds like it'll be doing something, rather than describing how we're rendering a set of URLs. but breadcrumbs are already an overloaded term here and don't want to 🚲 🏠

startTimestamp={startTimestamp}
isHovered={false}
isSelected={false}
onClick={() => {}}
Copy link
Member

Choose a reason for hiding this comment

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

Is this needed?

Copy link
Member Author

Choose a reason for hiding this comment

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

I was thinking to make the segments (and popup) inside the details page clickable, because in that spot we do have timestamps. On the list page i didn't ask for the api to return timestamps. We could ask for it, but i'd rather not throw new requirements in there at this point. So it's like a todo, keep or toss?

}

const Span = styled('span')`
color: ${p => p.theme.gray300};
Copy link
Member

Choose a reason for hiding this comment

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

Could this be

Suggested change
color: ${p => p.theme.gray300};
color: ${p => p.theme.subText};

`;

const Link = styled('a')`
color: ${p => p.theme.gray300};
Copy link
Member

Choose a reason for hiding this comment

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

same here?

Suggested change
color: ${p => p.theme.gray300};
color: ${p => p.theme.subText};

@Jesse-Box
Copy link
Contributor

hey @ryan953 looking good

  • The font used for the page walker needs to be our usual Rubik font-family
  • The chevron icon needs to be 12 by 12px
  • Tooltip underline should be visible when ... appear.

Heres the figma doc
https://www.figma.com/file/kAz36yY4I9siar93xBvce1/Specs%3A-Index-v2?node-id=51%3A4147

@ryan953
Copy link
Member Author

ryan953 commented Jul 28, 2022

The font used for the page walker needs to be our usual Rubik font-family

done

The chevron icon needs to be 12 by 12px

woah, looks way better!

Tooltip underline should be visible when ... appear.

I'm not sure how to do this tbh. The ... are coming from css so i don't have control over it in js. I'll google/stackoverflow around a bit, but it's gotta be timeboxed imo.

Screen Shot 2022-07-28 at 9 10 32 AM

Comment on lines +38 to +41
grid-template-columns: ${p =>
`minmax(auto, max-content) repeat(${
(p.cols - 2) * 2 + 1
}, max-content) minmax(auto, max-content)`};
Copy link
Member Author

Choose a reason for hiding this comment

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

This results in an extra template-column when there is only one segment to display.

This means the gap will still take up space on the right, but the 2nd column itself with have a width of 0.

@Jesse-Box
Copy link
Contributor

I'm not sure how to do this tbh. The ... are coming from css so i don't have control over it in js. I'll google/stackoverflow around a bit, but it's gotta be timeboxed imo.

yeah sure no worries

@ryan953 ryan953 changed the title feat(replays): Bootstrap the URLWalker component feat(replays): Create a "URLWalker" breadcrumb component Jul 28, 2022
@ryan953 ryan953 requested a review from billyvg July 28, 2022 16:38
@@ -86,7 +86,7 @@ function ReplayTable({replayList, idKey, showProjectColumn}: Props) {
email: replay['user.email'] ?? '',
}}
// this is the subheading for the avatar, so displayEmail in this case is a misnomer
displayEmail={getUrlPathname(replay.url) ?? ''}
displayEmail={<StringWalker urls={[]} />}
Copy link
Member Author

Choose a reason for hiding this comment

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

New api isn't part of this diff, so for now there are no urls on the index page (will say "0 Transactions" every time).

Copy link
Member

@billyvg billyvg left a comment

Choose a reason for hiding this comment

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

Let's give people a heads up that the index page will always say 0 pages until backend is in.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Scope: Frontend Automatically applied to PRs that change frontend components
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add the "Url Walker" component to the index page
3 participants