-
Notifications
You must be signed in to change notification settings - Fork 21
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
User Event Logs - filter, search and sorting #455
Conversation
return { data: paginatedResponse, loading: false }; | ||
}; | ||
|
||
export const useAllEventTypeLogs = (): { |
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.
let's rename useAllEventTypeLogs
to useAllEventLogTypes
|
||
const response = get( | ||
ApiAuthGet({ | ||
url: `/eventlogs/all${qs ? `?${qs}` : ''}`, |
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.
It might be a bit cleaner to use the apiAuthClient
instead of the ApiAuthGet
recoil selector -- then you can pass the query parameters as an object and you don't have to do this string formatting stuff.
I'm hoping we can rip out the recoil api selectors (ApiAuthGet
, ApiAuthPost
, etc.) entirely at some point.
eventLogsQueryParameters | ||
); | ||
|
||
const returnValues = useMemo( |
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.
getting a lint warning
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.
hm, i am not sure what to do here - if i add dependencies for useMemo hook (queryParameters & setQueryParameters) i will create infinite loop because we are updating queryParameters here and they will change every time.
Also i am not really sure why component is rerendering every time I update queryParameters @mattyg
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.
hm, i am not sure what to do here - if i add dependencies for useMemo hook (queryParameters & setQueryParameters) i will create infinite loop because we are updating queryParameters here and they will change every time. Also i am not really sure why component is rerendering every time I update queryParameters @mattyg
Sorry I think I didn't look closely enough at this the first time. I think the confusion comes from using this pattern where EventLogsList reads+writes to recoil state, and EventLogsActions also reads+writes to recoil state, and the parameters to the api request are stored in recoil state.
I think it would be clearer if instead:
- EventLogsList is the "smart" component, that reads+writes to recoil state
- The query parameters are stored internally in EventLogsList's component state
- EventLogsActions is a "dumb" component, that purely takes in user input and emits it to it's parent component. Or alternatively, you could remove it entirely and put the code inside it's parent component
Does that make sense? We could pair program it too if you want.
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.
Awesome!
I requested some minor changes inline.
The only other change I'd request is that we style the search, filter, and sort input components to reflect the styling used in the new Users page. Ideally by making generic input components we can keep using throughout the application.
ok.. i will have to check new user page and use those input styles |
…n & logs screen refactor
…k/praise into enh/user_event_logs
How about this:
We'll look into styling borders etc soon - perhaps making them slightly more soft. |
Great, almost there! I'll post a few comments before Monday. |
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.
Great! I made some minor styling changes. Merging now.
Resolves #376