-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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 #19068: While user in Pagination, Browser Back button takes them to start page #19229
base: main
Are you sure you want to change the base?
Changes from 5 commits
456d79f
a585f3a
d3539af
43e4b32
b6deece
f55eeeb
e31cdeb
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 | ||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -17,12 +17,21 @@ import { | |||||||||||||||||||||||||
useMemo, | ||||||||||||||||||||||||||
useState, | ||||||||||||||||||||||||||
} from 'react'; | ||||||||||||||||||||||||||
import { useHistory } from 'react-router-dom'; | ||||||||||||||||||||||||||
import { | ||||||||||||||||||||||||||
INITIAL_PAGING_VALUE, | ||||||||||||||||||||||||||
PAGE_SIZE_BASE, | ||||||||||||||||||||||||||
pagingObject, | ||||||||||||||||||||||||||
} from '../../constants/constants'; | ||||||||||||||||||||||||||
import { CursorType } from '../../enums/pagination.enum'; | ||||||||||||||||||||||||||
import { Paging } from '../../generated/type/paging'; | ||||||||||||||||||||||||||
import useCustomLocation from '../useCustomLocation/useCustomLocation'; | ||||||||||||||||||||||||||
|
||||||||||||||||||||||||||
interface CursorState { | ||||||||||||||||||||||||||
cursorType: CursorType | null; | ||||||||||||||||||||||||||
cursorValue: string | null; | ||||||||||||||||||||||||||
currentPage: number | null; | ||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||
|
||||||||||||||||||||||||||
export interface UsePagingInterface { | ||||||||||||||||||||||||||
paging: Paging; | ||||||||||||||||||||||||||
|
@@ -32,6 +41,12 @@ export interface UsePagingInterface { | |||||||||||||||||||||||||
pageSize: number; | ||||||||||||||||||||||||||
handlePageSizeChange: (page: number) => void; | ||||||||||||||||||||||||||
showPagination: boolean; | ||||||||||||||||||||||||||
storeCursor: (params: { | ||||||||||||||||||||||||||
cursorType: CursorType | null; | ||||||||||||||||||||||||||
cursorValue: string | null; | ||||||||||||||||||||||||||
currentPage: number | null; | ||||||||||||||||||||||||||
}) => void; | ||||||||||||||||||||||||||
getStoredCursor: () => CursorState; | ||||||||||||||||||||||||||
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. Let's follow same naming as above adn rename getter setter as below 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. done |
||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||
|
||||||||||||||||||||||||||
export const usePaging = ( | ||||||||||||||||||||||||||
|
@@ -40,6 +55,8 @@ export const usePaging = ( | |||||||||||||||||||||||||
const [paging, setPaging] = useState<Paging>(pagingObject); | ||||||||||||||||||||||||||
const [currentPage, setCurrentPage] = useState<number>(INITIAL_PAGING_VALUE); | ||||||||||||||||||||||||||
const [pageSize, setPageSize] = useState(defaultPageSize); | ||||||||||||||||||||||||||
const history = useHistory(); | ||||||||||||||||||||||||||
const location = useCustomLocation(); | ||||||||||||||||||||||||||
|
||||||||||||||||||||||||||
const handlePageSize = useCallback( | ||||||||||||||||||||||||||
(page: number) => { | ||||||||||||||||||||||||||
|
@@ -53,6 +70,39 @@ export const usePaging = ( | |||||||||||||||||||||||||
return paging.total > pageSize || pageSize !== defaultPageSize; | ||||||||||||||||||||||||||
}, [defaultPageSize, paging, pageSize]); | ||||||||||||||||||||||||||
|
||||||||||||||||||||||||||
const storeCursor = useCallback( | ||||||||||||||||||||||||||
({ | ||||||||||||||||||||||||||
cursorType, | ||||||||||||||||||||||||||
cursorValue, | ||||||||||||||||||||||||||
currentPage, | ||||||||||||||||||||||||||
}: { | ||||||||||||||||||||||||||
cursorType: CursorType | null; | ||||||||||||||||||||||||||
cursorValue: string | null; | ||||||||||||||||||||||||||
currentPage: number | null; | ||||||||||||||||||||||||||
}) => { | ||||||||||||||||||||||||||
history.replace({ | ||||||||||||||||||||||||||
state: { | ||||||||||||||||||||||||||
cursorType, | ||||||||||||||||||||||||||
cursorValue, | ||||||||||||||||||||||||||
currentPage, | ||||||||||||||||||||||||||
}, | ||||||||||||||||||||||||||
}); | ||||||||||||||||||||||||||
}, | ||||||||||||||||||||||||||
[] | ||||||||||||||||||||||||||
); | ||||||||||||||||||||||||||
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 function is not storing the cursor it's actually updating location state let's rename something to convey the same 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. done |
||||||||||||||||||||||||||
|
||||||||||||||||||||||||||
const getStoredCursor = () => { | ||||||||||||||||||||||||||
const cursorState = location.state as CursorState | undefined; | ||||||||||||||||||||||||||
|
||||||||||||||||||||||||||
return cursorState | ||||||||||||||||||||||||||
? { | ||||||||||||||||||||||||||
cursorType: cursorState.cursorType || null, | ||||||||||||||||||||||||||
cursorValue: cursorState.cursorValue || null, | ||||||||||||||||||||||||||
currentPage: cursorState.currentPage || null, | ||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||
: { cursorType: null, cursorValue: null, currentPage: null }; | ||||||||||||||||||||||||||
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.
Suggested change
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. done |
||||||||||||||||||||||||||
}; | ||||||||||||||||||||||||||
|
||||||||||||||||||||||||||
return { | ||||||||||||||||||||||||||
paging, | ||||||||||||||||||||||||||
handlePagingChange: setPaging, | ||||||||||||||||||||||||||
|
@@ -61,5 +111,7 @@ export const usePaging = ( | |||||||||||||||||||||||||
pageSize, | ||||||||||||||||||||||||||
handlePageSizeChange: handlePageSize, | ||||||||||||||||||||||||||
showPagination: paginationVisible, | ||||||||||||||||||||||||||
storeCursor, | ||||||||||||||||||||||||||
getStoredCursor, | ||||||||||||||||||||||||||
}; | ||||||||||||||||||||||||||
}; |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -56,6 +56,7 @@ import { | |
EntityType, | ||
TabSpecificField, | ||
} from '../../enums/entity.enum'; | ||
import { CursorType } from '../../enums/pagination.enum'; | ||
import { CreateThread } from '../../generated/api/feed/createThread'; | ||
import { Tag } from '../../generated/entity/classification/tag'; | ||
import { DatabaseSchema } from '../../generated/entity/data/databaseSchema'; | ||
|
@@ -100,6 +101,8 @@ const DatabaseSchemaPage: FunctionComponent = () => { | |
handlePagingChange, | ||
currentPage, | ||
handlePageChange, | ||
storeCursor, | ||
getStoredCursor, | ||
} = pagingInfo; | ||
|
||
const { tab: activeTab = EntityTabs.TABLE } = | ||
|
@@ -461,10 +464,15 @@ const DatabaseSchemaPage: FunctionComponent = () => { | |
({ cursorType, currentPage }: PagingHandlerParams) => { | ||
if (cursorType) { | ||
getSchemaTables({ [cursorType]: paging[cursorType] }); | ||
storeCursor({ | ||
cursorType: cursorType, | ||
cursorValue: paging[cursorType as CursorType]!, | ||
currentPage: currentPage, | ||
}); | ||
} | ||
handlePageChange(currentPage); | ||
}, | ||
[paging, getSchemaTables] | ||
[paging, getSchemaTables, storeCursor, handlePageChange] | ||
); | ||
|
||
const versionHandler = useCallback(() => { | ||
|
@@ -520,8 +528,19 @@ const DatabaseSchemaPage: FunctionComponent = () => { | |
}, [viewDatabaseSchemaPermission]); | ||
|
||
useEffect(() => { | ||
const cursorState = getStoredCursor(); | ||
if (viewDatabaseSchemaPermission && decodedDatabaseSchemaFQN) { | ||
getSchemaTables({ limit: pageSize }); | ||
if (cursorState.cursorType) { | ||
// Fetch data if cursorType is present in state with cursor Value to handle browser back navigation | ||
getSchemaTables({ | ||
[cursorState.cursorType]: cursorState.cursorValue, | ||
limit: pageSize, | ||
}); | ||
handlePageChange(cursorState.currentPage as number); | ||
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 think we can pass cursor value to page handler and make it work as optional param so if we want to store it we can pass it else it won't be in effect 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. Just to clarify once, do you mean:
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 pointing at this: 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. handlePageChange is simply a setter function for currentPage in usePaging |
||
} else { | ||
// Otherwise, just fetch the data without cursor value | ||
getSchemaTables({ limit: pageSize }); | ||
} | ||
} | ||
}, [ | ||
showDeletedTables, | ||
|
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.
currentPage is already there so no need to pull it here sepratly