-
Notifications
You must be signed in to change notification settings - Fork 121
Fix first schema check landing width & sidenav loading flicker #7066
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
Changes from all commits
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 |
---|---|---|
@@ -0,0 +1,5 @@ | ||
--- | ||
'hive': patch | ||
--- | ||
|
||
Fix first schema check landing width; fix flickering sidenav on schema check loading" |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,4 +1,4 @@ | ||
import { useCallback, useMemo, useState } from 'react'; | ||
import { Fragment, useCallback, useMemo, useState } from 'react'; | ||
import { format } from 'date-fns'; | ||
import { BadgeCheck, ChevronDown, GitCompareIcon, Loader2 } from 'lucide-react'; | ||
import { useMutation, useQuery } from 'urql'; | ||
|
@@ -281,20 +281,20 @@ function ConditionalBreakingChangesMetadataSection(props: { | |
{numberOfTargets <= 3 && ( | ||
<> | ||
{allTargets.map((target, index) => ( | ||
<> | ||
<Fragment key={target.slug}> | ||
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. fixes a react |
||
<span className="text-white">{target.slug}</span> | ||
{index === allTargets.length - 1 ? null : ', '} | ||
</> | ||
</Fragment> | ||
))} | ||
</> | ||
)} | ||
{numberOfTargets > 3 && ( | ||
<> | ||
{truncatedTargets.map((target, index) => ( | ||
<> | ||
<Fragment key={target.slug}> | ||
<span className="text-white">{target.slug}</span> | ||
{index === truncatedTargets.length - 1 ? null : ', '} | ||
</> | ||
</Fragment> | ||
))} | ||
{' and '} | ||
<Popover> | ||
|
@@ -319,7 +319,7 @@ function ConditionalBreakingChangesMetadataSection(props: { | |
</PopoverContent> | ||
</Popover> | ||
</> | ||
)}{' '} | ||
)} | ||
. <br /> | ||
Usage data ranges from{' '} | ||
<span className="text-white"> | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,4 +1,4 @@ | ||
import { useState } from 'react'; | ||
import { useEffect, useRef, useState } from 'react'; | ||
import { useQuery } from 'urql'; | ||
import { Page, TargetLayout } from '@/components/layouts/target'; | ||
import { BadgeRounded } from '@/components/ui/badge'; | ||
|
@@ -230,7 +230,6 @@ function ChecksPageContent(props: { | |
const [paginationVariables, setPaginationVariables] = useState<Array<string | null>>(() => [ | ||
null, | ||
]); | ||
const [hasSchemaChecks, setHasSchemaChecks] = useState(false); | ||
const navigate = useNavigate(); | ||
const { schemaCheckId } = useParams({ | ||
strict: false /* allows to read the $schemaCheckId param of its child route */, | ||
|
@@ -266,9 +265,17 @@ function ChecksPageContent(props: { | |
); | ||
} | ||
|
||
if (!hasSchemaChecks && !!query.data?.target?.schemaChecks?.edges?.length) { | ||
setHasSchemaChecks(true); | ||
} | ||
const [isLoading] = useDebouncedLoader(query.fetching || query.stale); | ||
|
||
const [hasSchemaChecks, setHasSchemaChecks] = useState( | ||
!!query.data?.target?.schemaChecks?.edges?.length, | ||
); | ||
useEffect(() => { | ||
if (!query.stale && !query.fetching) { | ||
setHasSchemaChecks(!!query.data?.target?.schemaChecks?.edges?.length); | ||
} | ||
}, [query.fetching, query.stale, !query.data?.target?.schemaChecks?.edges?.length]); | ||
|
||
const hasFilteredSchemaChecks = !!query.data?.target?.filteredSchemaChecks?.edges?.length; | ||
const hasActiveSchemaCheck = !!schemaCheckId; | ||
|
||
|
@@ -290,9 +297,11 @@ function ChecksPageContent(props: { | |
}); | ||
}; | ||
|
||
const loadMore = (cursor: string) => setPaginationVariables(cursors => [...cursors, cursor]); | ||
|
||
return ( | ||
<> | ||
<div> | ||
<div className={cn(!hasActiveSchemaCheck && !hasSchemaChecks && 'w-full')}> | ||
<div className="w-[300px] py-6"> | ||
<Title>Schema Checks</Title> | ||
<Subtitle>Recently checked schemas.</Subtitle> | ||
|
@@ -337,40 +346,46 @@ function ChecksPageContent(props: { | |
schemaCheckId={schemaCheckId} | ||
after={cursor} | ||
isLastPage={index + 1 === paginationVariables.length} | ||
onLoadMore={cursor => setPaginationVariables(cursors => [...cursors, cursor])} | ||
onLoadMore={loadMore} | ||
key={cursor ?? 'first'} | ||
showOnlyChanged={showOnlyChanged} | ||
showOnlyFailed={showOnlyFailed} | ||
/> | ||
))} | ||
</div> | ||
) : query.fetching || query.stale ? ( | ||
<Spinner /> | ||
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. moved outside this ternary so I could better control its rendering with the other elements. 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. Actually the whole ternaries look too bloated; maybe we should consider constructing the JSX parts outside of the return statement then combine 🫠 |
||
) : ( | ||
<div className="my-4 cursor-default text-center text-sm text-gray-400"> | ||
No schema checks found with the current filters | ||
</div> | ||
!query.fetching && | ||
!query.stale && ( | ||
<div className="my-4 cursor-default text-center text-sm text-gray-400"> | ||
No schema checks found with the current filters | ||
</div> | ||
) | ||
)} | ||
</div> | ||
) : query.fetching ? ( | ||
<Spinner /> | ||
) : ( | ||
<div> | ||
<div className="cursor-default text-sm"> | ||
{hasActiveSchemaCheck ? ( | ||
'List is empty' | ||
) : ( | ||
<NoSchemaVersion | ||
projectType={query.data?.target?.project.type ?? null} | ||
recommendedAction="check" | ||
/> | ||
)} | ||
!query.fetching && | ||
!query.stale && ( | ||
<div> | ||
<div className="cursor-default text-sm"> | ||
{!hasActiveSchemaCheck && ( | ||
<NoSchemaVersion | ||
projectType={query.data?.target?.project.type ?? null} | ||
recommendedAction="check" | ||
/> | ||
)} | ||
</div> | ||
<DocsLink | ||
href="/features/schema-registry#check-a-schema" | ||
className="flex flex-row items-center" | ||
> | ||
Learn how to check your first schema | ||
</DocsLink> | ||
</div> | ||
<DocsLink href="/features/schema-registry#check-a-schema"> | ||
{hasActiveSchemaCheck | ||
? 'Check you first schema' | ||
: 'Learn how to check your first schema with Hive CLI'} | ||
</DocsLink> | ||
) | ||
)} | ||
{isLoading && ( | ||
<div className="mt-4 flex w-full grow flex-col items-center"> | ||
<Spinner /> | ||
</div> | ||
)} | ||
</div> | ||
|
@@ -410,3 +425,28 @@ export function TargetChecksPage(props: { | |
</> | ||
); | ||
} | ||
|
||
const useDebouncedLoader = (isLoading: boolean, delay = 500) => { | ||
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 also looked at using the 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. Haven't deeply looked into, but looks like this lib offers a variety of options for scheduling 👀 Not sure whether it's worth adding a dep though |
||
const [showLoadingIcon, setShowLoadingIcon] = useState(false); | ||
const timerRef = useRef<ReturnType<typeof setTimeout> | undefined>(undefined); | ||
|
||
useEffect(() => { | ||
if (isLoading) { | ||
// Start a timer to show the loading icon after the delay | ||
timerRef.current = setTimeout(() => { | ||
setShowLoadingIcon(true); | ||
}, delay); | ||
} else { | ||
// If loading finishes, clear any pending timer and hide the icon | ||
clearTimeout(timerRef.current); | ||
setShowLoadingIcon(false); | ||
} | ||
|
||
// Cleanup function to clear the timer on unmount or if isLoading changes | ||
return () => { | ||
clearTimeout(timerRef.current); | ||
}; | ||
}, [isLoading, delay]); | ||
|
||
return [showLoadingIcon]; | ||
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. Doesn't seem to have to be a tuple 😅 |
||
}; |
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.
opted to hide for checks since there is a separate link to docs for them