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.

Sign up for GitHub

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

Jump to bottom

Feat: tree list query limit by time #412

Merged
merged 6 commits into from
Oct 23, 2024
Merged

Feat: tree list query limit by time #412

merged 6 commits into from
Oct 23, 2024

Conversation

Francisco2002
Copy link
Collaborator

Add an input to allow the user to provide a time parameter in the tree listing page.

How to test:

  1. go to http://localhost:5173/tree and interact with the input.
  2. go to http://localhost:5173/tree?origin=maestro&time=... and try to pass a non-positive value in time param.

#88

@Francisco2002 Francisco2002 changed the title Feat/tree list query limit by time Feat: tree list query limit by time Oct 21, 2024
@Francisco2002 Francisco2002 marked this pull request as ready for review October 21, 2024 14:27
@Francisco2002 Francisco2002 linked an issue Oct 21, 2024 that may be closed by this pull request
make it possible to change the query limit by time in tree listing page #88
Closed
MarceloRobert
MarceloRobert reviewed Oct 21, 2024
View reviewed changes
WilsonNet
WilsonNet reviewed Oct 21, 2024
View reviewed changes
dashboard/src/pages/treeConstants.ts Outdated
@@ -0,0 +1,2 @@
export const DEBOUNCE_INTERVAL = 666;
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think there is any specific reason for this to be a global costat, maybe it could be the default debounce interval for the text input if the user didn't specify any

WilsonNet
WilsonNet reviewed Oct 21, 2024
View reviewed changes
dashboard/src/hooks/use-toast.ts Outdated
const [state, setState] = React.useState<State>(memoryState);

React.useEffect(() => {
listeners.push(setState);
Copy link
Collaborator

Choose a reason for hiding this comment

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

wow this is so hacky, I know it is from shadcn, lets keep this as is since it is working

WilsonNet
WilsonNet reviewed Oct 21, 2024
View reviewed changes
dashboard/src/components/TreeListingPage/TreeListingPage.tsx Outdated
}): JSX.Element => {
const className = useMemo(
() =>
`border-${fieldError ? 'red' : 'gray'} mx-[10px] flex w-[100px] flex-1 rounded border`,
Copy link
Collaborator

Choose a reason for hiding this comment

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

border-${fieldError ? 'red' : 'gray'}

will bug the tailwind compiler, remember that it interprets the test value not the javascript

https://tailwindcss.com/docs/content-configuration#dynamic-class-names

Copy link
Collaborator

Choose a reason for hiding this comment

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

use the cn utils for it

@Francisco2002 Francisco2002 force-pushed the feat/tree-list-query-limit-by-time branch from 1fd2256 to fbc7ebe Compare October 22, 2024 13:57
MarceloRobert
MarceloRobert approved these changes Oct 22, 2024
View reviewed changes
Copy link
Collaborator

@MarceloRobert MarceloRobert left a comment

Choose a reason for hiding this comment

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

Looks good to me

WilsonNet
WilsonNet reviewed Oct 23, 2024
View reviewed changes
dashboard/src/components/DebounceInput/DebounceInput.tsx Outdated
debouncedSideEffect,
startingValue = '',
onChange,
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this is a leftover, you are not using in the TreeListingPage and it feels weird since we have the debouncedSideEffect

WilsonNet
WilsonNet reviewed Oct 23, 2024
View reviewed changes
dashboard/src/components/TreeListingPage/TreeListingPage.tsx Outdated
Comment on lines 63 to 67
function validateString(val: string): boolean {
const convertedNumber = parseInt(val);
return !Number.isNaN(convertedNumber) && convertedNumber > 0;
}

Copy link
Collaborator

Choose a reason for hiding this comment

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

since this is a pure function you can leave outside of the component so we don't have to worry about memoization or keeping an stable reference

WilsonNet
WilsonNet reviewed Oct 23, 2024
View reviewed changes
dashboard/src/components/ui/form.tsx Outdated
@@ -0,0 +1,176 @@
import * as React from "react"
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think you never used this file, you can delete it

WilsonNet
WilsonNet reviewed Oct 23, 2024
View reviewed changes
dashboard/src/components/ui/label.tsx Outdated
@@ -0,0 +1,24 @@
import * as React from "react"
Copy link
Collaborator

Choose a reason for hiding this comment

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

since you are not using form, you can delete the label component also

WilsonNet
WilsonNet approved these changes Oct 23, 2024
View reviewed changes
Copy link
Collaborator

@WilsonNet WilsonNet left a comment

Choose a reason for hiding this comment

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

it is working really nice, just some few pointers to go

lfjnascimento
lfjnascimento reviewed Oct 23, 2024
View reviewed changes
backend/kernelCI_app/views/treeView.py Outdated
Comment on lines 14 to 15
time = int(request.GET.get('time', '7'))
timedata = {"days": time}
Copy link
Collaborator

@lfjnascimento lfjnascimento Oct 23, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I think we should receive it in a more standard way such as a unix timestamp or a ISO 8601

lfjnascimento
lfjnascimento reviewed Oct 23, 2024
View reviewed changes
backend/requests/tree-listing-fast.sh Outdated
@@ -1,3 +1,6 @@
# If you want to see headers, add -p H to the http call:
# http -p H 'http://localhost:8000/api/tree/' origin==maestro
http 'http://localhost:8000/api/tree-fast/' origin==maestro

# If you want to provide another limit to query:
# http 'http://localhost:8000/api/tree-fast/' origin==maestro value==8
Copy link
Collaborator

Choose a reason for hiding this comment

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

is this value right? shouldn't it be time?

@Francisco2002 Francisco2002 force-pushed the feat/tree-list-query-limit-by-time branch from fbc7ebe to ac14c78 Compare October 23, 2024 18:26
@Francisco2002 Francisco2002 force-pushed the feat/tree-list-query-limit-by-time branch from ac14c78 to 5327c13 Compare October 23, 2024 19:05
@Francisco2002 Francisco2002 merged commit 48e1e04 into main Oct 23, 2024
5 checks passed
@Francisco2002 Francisco2002 deleted the feat/tree-list-query-limit-by-time branch October 24, 2024 16:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lfjnascimento lfjnascimento lfjnascimento left review comments

@WilsonNet WilsonNet WilsonNet approved these changes

@MarceloRobert MarceloRobert MarceloRobert approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

make it possible to change the query limit by time in tree listing page
4 participants
@Francisco2002 @WilsonNet @lfjnascimento @MarceloRobert