-
Notifications
You must be signed in to change notification settings - Fork 8
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
fix: Ensure issuesGroupedState is up to date #348
fix: Ensure issuesGroupedState is up to date #348
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
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.
self review and comments
@@ -114,7 +105,7 @@ export function RoadmapDetailed({ | |||
|
|||
return innerDayjsDates; | |||
// eslint-disable-next-line react-hooks/exhaustive-deps | |||
}, [issuesGroupedState.length, issuesGroupedId]); | |||
}, [issuesGroupedState, issuesGroupedId]); |
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.
useHookstateMemo properly recognizes uniqueness of hookState objects.
setIssuesGroupedState(() => convertIssueDataStateToDetailedViewGroupOld(issueDataState, viewMode, query)) | ||
} | ||
}, [viewMode, query, setIssuesGroupedState, issueDataState, groupedIssuesIdPrev, groupedIssuesId]); | ||
const memoGrouped = useHookstateMemo( |
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.
useHookstateMemo properly recognizes uniqueness of hookState objects.
const invalidGroups = issuesGroupedState.filter((group) => group.ornull == null || group.items.ornull == null) | ||
if (issuesGroupedState.value.length === 0 || invalidGroups.length > 0) { | ||
if (invalidGroups.length > 0) { | ||
invalidGroups.forEach((g) => { | ||
console.warn('Found an invalid group: ', g.value); | ||
}); | ||
} | ||
return <Spinner />; | ||
} |
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.
only processing invalidGroups after globalLoadingState check
const issuesGroupedState = useHookstate<DetailedViewGroup[]>([]); | ||
const groupedIssuesId = getUniqIdForGroupedIssues(issuesGroupedState.value) | ||
const groupedIssuesIdPrev = usePrevious(groupedIssuesId); | ||
const globalLoadingState = useGlobalLoadingState(); |
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.
moving global loading state up to the top
@@ -71,10 +71,9 @@ export function RoadmapTabbedView({ | |||
} | |||
|
|||
return ( | |||
<Skeleton isLoaded={!globalLoadingState.get()}> | |||
<Skeleton isLoaded={!globalLoadingState.get()} key={index}> |
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.
fix react warning
@@ -33,13 +33,12 @@ export function GridRow({ | |||
const viewMode = useViewMode(); | |||
const routerQuery = useRouter().query; | |||
const [closestDateIdx, setClosestDateIdx] = useState(Math.round(globalTimeScaler.getColumn(dayjs.utc(milestone.due_date.get()).toDate()))); | |||
useEffect(() => { | |||
useHookstateEffect(() => { |
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.
note: useHookstateEffect is supposed to recognize hookstate objects, but doesn't seem to work properly, unlike useHookstateMemo (which does work properly, not rerunning the hook callback if hookstate object wasn't changed)
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.
LGTM
@@ -5,7 +5,7 @@ const SvgDetailViewIcon = () => ( | |||
* From https://www.iconfinder.com/icons/326721/list_view_icon#svg | |||
*/ | |||
<svg width="19" height="14" viewBox="0 0 19 14" xmlns="http://www.w3.org/2000/svg" version="1.1"> | |||
<g fill="none" fill-rule="evenodd" id="Page-1" stroke="#fff" stroke-width="1"> | |||
<g fill="none" fillRule="evenodd" id="Page-1" stroke="#fff" strokeWidth="1"> |
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.
🤔 Did this API change?
invalidGroups.forEach((g) => { | ||
console.warn('Found an invalid group: ', g.value); | ||
}); |
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.
optionally
invalidGroups.forEach((g) => { | |
console.warn('Found an invalid group: ', g.value); | |
}); | |
invalidGroups.forEach(({value}) => console.warn('Found an invalid group: ', value)); |
fixes #347
demo url: https://starmaps-git-347-bug-detail-view-not-loading-c9426c-ipfs-ignite.vercel.app/roadmap/github.com/protocol/engres/issues/9#detail