placeholder: cleanup placeholder API #102999
Draft
Conversation
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
github-actions
bot
added
the
Scope: Frontend
❌ 2 Tests Failed:
View the top 2 failed test(s) by shortest run time
To view more test analytics, go to the Test Analytics Dashboard |
JonasBa
commented
Nov 8, 2025
| }; | ||
|
|
||
| export function ActivityAvatar({className, type, user, size = 38}: Props) { | ||
| export function ActivityAvatar({type, user, size = 38}: Props) { |
Member
Author
There was a problem hiding this comment.
There are no more usages of styled activity avatar now
JonasBa
commented
Nov 8, 2025
| shape="circle" | ||
| /> | ||
| ); | ||
| return <Placeholder width={`${size}px`} height={`${size}px`} shape="circle" />; |
Member
Author
There was a problem hiding this comment.
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
JonasBa
commented
Nov 8, 2025
| return <Component {...props} chartRef={props.ref} />; | ||
| } | ||
|
|
||
| const ErrorPlaceholder = styled(Placeholder)` |
Member
Author
There was a problem hiding this comment.
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
JonasBa
commented
Nov 8, 2025
| 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.
Our sizing scale does not support 20px, so this is now 16px.
JonasBa
commented
Nov 8, 2025
Comment on lines
8
to
+9
| children?: React.ReactNode; | ||
| className?: string; | ||
| error?: React.ReactNode; | ||
| height?: string; | ||
| shape?: 'rect' | 'circle'; | ||
| style?: React.CSSProperties; | ||
| testId?: string; | ||
| width?: string; | ||
| } | ||
| shape?: 'circle'; |
Member
Author
There was a problem hiding this comment.
Next step is to get it to this
Suggested change
| children?: React.ReactNode; | |
| className?: string; | |
| error?: React.ReactNode; | |
| height?: string; | |
| shape?: 'rect' | 'circle'; | |
| style?: React.CSSProperties; | |
| testId?: string; | |
| width?: string; | |
| } | |
| shape?: 'circle'; | |
| children?: never; |
JonasBa
commented
Nov 8, 2025
| background="tertiary" | ||
| width={props.width ?? '100%'} | ||
| height={props.height ?? '60px'} | ||
| radius={shape === 'circle' ? 'full' : (radius ?? 'md')} |
Member
Author
There was a problem hiding this comment.
circle is just another way of saying full, and I don't like either of them
JonasBa
commented
Nov 8, 2025
| [location.pathname, navigate] | ||
| ); | ||
|
|
||
| const renderEmpty = useCallback((placeholderText = '') => { |
JonasBa
commented
Nov 8, 2025
| <SimpleTable.RowCell> | ||
| <div style={{width: '100%'}}> | ||
| <Placeholder height="20px" width="50%" style={{marginBottom: '4px'}} /> | ||
| <ScrapsContainer marginBottom="xs"> |
Member
Author
There was a problem hiding this comment.
There is a container component defined in this file, hence the alias
JonasBa
commented
Nov 8, 2025
| const LoadingPlaceholder = styled(({className}: PlaceholderProps) => ( | ||
| <Placeholder height="200px" className={className} /> | ||
| ))` | ||
| background-color: ${p => p.theme.surface300}; |
Member
Author
There was a problem hiding this comment.
The placeholder tertiary should be fine here
JonasBa
commented
Nov 8, 2025
| return ( | ||
| <Container> | ||
| <Placeholder height="100%" error={t('Error loading chart')} /> | ||
| <Container height="100%"> |
Member
Author
There was a problem hiding this comment.
Not sure why this container is needed when the Placeholder is a div with 100%, but I've just dropped the styled component here and used the Container from layout
JonasBa
commented
Nov 8, 2025
| /> | ||
| ) : ( | ||
| <Placeholder | ||
| shape="rect" |
JonasBa
commented
Nov 8, 2025
| ) : null} | ||
| </div> | ||
| <Placeholder | ||
| className="Placeholder" |
JonasBa
commented
Nov 8, 2025
|
|
||
| const handleSort = (field: 'name') => { | ||
| const {cursor, navigation, ...queryWithoutPagination} = location.query; | ||
| const {_cursor, _navigation, ...queryWithoutPagination} = location.query; |
Member
Author
There was a problem hiding this comment.
Not sure how this got committed, but the linter wasn't happy about unused variables here
ryan953
changed the title
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Refactor Placeholder to use the new Container API and drop majority of the props. I will followup with removing of the children as well, as this component should not be used as a replacement for empty states.