-
-
Notifications
You must be signed in to change notification settings - Fork 4.5k
placeholder: cleanup placeholder API #102999
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
base: master
Are you sure you want to change the base?
Changes from all commits
ff29a4e
fae7844
e17eac7
7bc884b
266069f
82a0ba9
f2c00a0
32a08f0
1efac14
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,39 +1,31 @@ | ||
| import styled from '@emotion/styled'; | ||
|
|
||
| import {UserAvatar} from 'sentry/components/core/avatar/userAvatar'; | ||
| import Placeholder from 'sentry/components/placeholder'; | ||
| import {Placeholder} from 'sentry/components/placeholder'; | ||
| import {IconSentry} from 'sentry/icons'; | ||
| import type {AvatarUser} from 'sentry/types/user'; | ||
|
|
||
| type Props = { | ||
| type: 'system' | 'user'; | ||
| className?: string; | ||
| size?: number; | ||
| user?: AvatarUser; | ||
| }; | ||
|
|
||
| export function ActivityAvatar({className, type, user, size = 38}: Props) { | ||
| export function ActivityAvatar({type, user, size = 38}: Props) { | ||
| if (user) { | ||
| return <UserAvatar user={user} size={size} className={className} />; | ||
| return <UserAvatar user={user} size={size} />; | ||
| } | ||
|
|
||
| if (type === 'system') { | ||
| // Return Sentry avatar | ||
| return ( | ||
| <SystemAvatar className={className} size={size}> | ||
| <SystemAvatar size={size}> | ||
| <StyledIconSentry size="md" /> | ||
| </SystemAvatar> | ||
| ); | ||
| } | ||
|
|
||
| return ( | ||
| <Placeholder | ||
| className={className} | ||
| width={`${size}px`} | ||
| height={`${size}px`} | ||
| shape="circle" | ||
| /> | ||
| ); | ||
| return <Placeholder width={`${size}px`} height={`${size}px`} shape="circle" />; | ||
|
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I am not a fan of the shape prop. I may just remove it in favor of radius="full" so it follows the Container component convention |
||
| } | ||
|
|
||
| type SystemAvatarProps = { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,7 +1,8 @@ | ||
| import styled from '@emotion/styled'; | ||
| import {captureException} from '@sentry/core'; | ||
| import {useQuery} from '@tanstack/react-query'; | ||
|
|
||
| import Placeholder from 'sentry/components/placeholder'; | ||
| import {Placeholder} from 'sentry/components/placeholder'; | ||
| import {t} from 'sentry/locale'; | ||
| import type {ReactEchartsRef} from 'sentry/types/echarts'; | ||
| import type {LoadableChartWidgetProps} from 'sentry/views/insights/common/components/widgets/types'; | ||
|
|
@@ -228,8 +229,13 @@ export function ChartWidgetLoader(props: Props) { | |
| // eslint-disable-next-line no-console | ||
| console.error(error); | ||
| captureException(error); | ||
| return <Placeholder height="100%" error={t('Error loading widget')} />; | ||
| return <ErrorPlaceholder height="100%">{t('Error loading widget')}</ErrorPlaceholder>; | ||
| } | ||
|
|
||
| return <Component {...props} chartRef={props.ref} />; | ||
| } | ||
|
|
||
| const ErrorPlaceholder = styled(Placeholder)` | ||
|
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is not what we should be using the component for. Given that there were only two instances where this was setup, I've went ahead and made these custom styled. At some point in the future, we will review all usages of styled(CoreComponent), and look at this in more detail, but for now, this is at least not something that we support ootb |
||
| color: ${p => p.theme.red200}; | ||
| background-color: ${p => p.theme.red100}; | ||
| `; | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,7 +1,9 @@ | ||
| import React from 'react'; | ||
|
|
||
| import {Container} from '@sentry/scraps/layout'; | ||
|
|
||
| import {useEventGroupingInfo} from 'sentry/components/events/groupingInfo/useEventGroupingInfo'; | ||
| import Placeholder from 'sentry/components/placeholder'; | ||
| import {Placeholder} from 'sentry/components/placeholder'; | ||
| import {t} from 'sentry/locale'; | ||
| import type {Event} from 'sentry/types/event'; | ||
| import type {Group} from 'sentry/types/group'; | ||
|
|
@@ -34,11 +36,9 @@ export function GroupInfoSummary({ | |
|
|
||
| if (isPending && !hasPerformanceGrouping) { | ||
| return ( | ||
| <Placeholder | ||
| height="20px" | ||
| width="unset" | ||
| style={{flexGrow: 1, marginBottom: '20px'}} | ||
| /> | ||
| <Container marginBottom="xl"> | ||
| {props => <Placeholder height="20px" width="unset" flexGrow={1} {...props} />} | ||
|
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Our sizing scale does not support 20px, so this is now 16px. |
||
| </Container> | ||
| ); | ||
| } | ||
|
|
||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are no more usages of styled activity avatar now