Skip to content
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

Parent table with params #218

Merged
merged 4 commits into from Jun 10, 2022
Merged

Conversation

anncatton
Copy link
Contributor

@anncatton anncatton commented Jun 1, 2022

Adds parameters to list context

  • adds back sorting, search and pagination
  • fixes table sort. the api results were in the correct order, but the table logic was reversing the results, wheeee

Base automatically changed from 207-parent-table to 161-replace-freactal June 1, 2022 15:17
@anncatton anncatton marked this pull request as ready for review June 1, 2022 15:33
setListState(data);
}
}, [resourceName, currentListParams]);
const setListParams = useMemo(

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not sure why we're using useMemo here?


// from https://usehooks.com/useDebounce/

const useDebounce = <T,>(value: T, delay: number): T => {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

<T,> ? weird. prettier checks out though

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah i just lazily copied it...we have it in dac-ui too 🤷‍♀️

@@ -148,7 +152,9 @@ const List = () => {
backgroundColor: 'transparent',
...(listParams.sortOrder === 'DESC' && { color: theme.colors.primary_5 }),
}}
// onClick={() => setListParams({ ...listParams, sortOrder: 'DESC' })}
onClick={() =>
setListParams({ sortOrder: listParams.sortOrder === 'ASC' ? 'DESC' : 'ASC' })

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

worth putting 'ASC' and 'DESC' into enum? used a fair bit

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Funny I thought I did have one, and forgot to use it here...but will do!

Copy link
Contributor Author

@anncatton anncatton Jun 9, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nvmd i def do, it was just a type instead of an enum...will use it here!

Copy link

@ciaranschutte ciaranschutte left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm
one suggestion if you fancy

@anncatton anncatton merged commit 6c6a6b3 into 161-replace-freactal Jun 10, 2022
@anncatton anncatton deleted the parent-table-with-params branch June 10, 2022 13:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants