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

feat(HardwareDetails): add platform card to tabs #710

Merged
merged 3 commits into from
Dec 24, 2024
Merged

Conversation

murilx
Copy link
Contributor

@murilx murilx commented Dec 23, 2024

Closes #678

How to test

  • Add platform filter for one tab and verify that the values match
  • With a filter selected for one tab, try changing tabs
  • Add filter for multiple tabs and verify that all values correctly match
  • With a filter selected, try going for a test or build details and verify that coming back to hardware details keep the filters
  • Add other filters and verify that they work correctly
  • Try doing the same steps in tree details and verify that the behaviour is still the same (this is recommended since some filter methods that are used in tree details were changed)

Example of Hardware Details with more than one platform (localhost)

Visual Reference

Builds tab

Boots tab

Tests tab

@murilx murilx self-assigned this Dec 23, 2024
@murilx murilx force-pushed the feat/platforms-card branch 2 times, most recently from 1d02243 to 9a2d212 Compare December 23, 2024 16:47
@murilx murilx marked this pull request as ready for review December 23, 2024 17:59
class ParsedFilter(TypedDict):
field: str
value: Any # TODO: correctly type this field
comparison_op: Literal["exact", "in", "gt", "gte", "lt", "lte", "like"]
Copy link
Collaborator

Choose a reason for hiding this comment

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

nice

@@ -115,6 +128,11 @@ const TestsTab = ({ tests, trees, hardwareId }: TTestsTab): JSX.Element => {
configStatusCounts={tests.configs}
diffFilter={diffFilter}
/>
<MemoizedPlatformsCard
Copy link
Collaborator

Choose a reason for hiding this comment

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

because everything is in the same div, everything is on the same column can you spread the cards between two divs here?

@@ -102,6 +110,11 @@ const BootsTab = ({ boots, hardwareId, trees }: TBootsTab): JSX.Element => {
configStatusCounts={boots.configs}
diffFilter={diffFilter}
/>
<MemoizedPlatformsCard
Copy link
Collaborator

Choose a reason for hiding this comment

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

ditto about two divs

@WilsonNet
Copy link
Collaborator

filters are missing in the modals

@murilx murilx force-pushed the feat/platforms-card branch 2 times, most recently from 3b7e635 to 24f2277 Compare December 24, 2024 11:59
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.

See if you can combine the second with the first commit. You could also split the commits into backend/frontend. The third commit is ok, it's just formatting.

@murilx murilx force-pushed the feat/platforms-card branch from 24f2277 to 0142686 Compare December 24, 2024 14:18
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.

Worked on my tests, LGTM

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.

working nice here

- Changed `'` to `"` as it's been used through all the source code
- Changed identation in function arguments and boolean expressions
@WilsonNet WilsonNet merged commit a154a68 into main Dec 24, 2024
5 checks passed
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.

Know which board is compatible [Talk with KernelCI folks]
3 participants