-
Notifications
You must be signed in to change notification settings - Fork 3
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
Test execution search and query params #141
Conversation
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.
LGTM! No complains about the code, but I will let @nadzyah also review the code and +1 it 😄
@@ -13,38 +13,38 @@ class FindShortcut extends ConsumerWidget { | |||
|
|||
@override | |||
Widget build(BuildContext context, WidgetRef ref) { | |||
final shouldActivateShortcut = AppRoutes.isAtDashboardPage(context); | |||
final shouldMakeFiltersVisible = | |||
AppRoutes.isDashboardPage(AppRoutes.uriFromContext(context)); |
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.
Just curious, do we want to bind ctrl+f to the search bar on each artefact page, or just in the dashboard page?
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.
ctrl+f does work on both pages now. This condition is to display the hidden filters on dashboard page
return emptyArtefactFilters | ||
.copyWithOptionsExtracted(artefacts) | ||
.copyWithQueryParams(pageUri.queryParametersAll); | ||
} else if (AppRoutes.isArtefactPage(pageUri)) { |
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.
I guess you don't need an else
statement after return
return emptyTestExecutionFilters | ||
.copyWithOptionsExtracted(testExecutions) | ||
.copyWithQueryParams(pageUri.queryParametersAll); | ||
} else { |
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.
Same here
frontend/lib/routing.dart
Outdated
return uri.pathSegments[1].toInt(); | ||
} else { |
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.
And here?
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.
Thanks for the opportunity to review the PR!
Resolves RTW-244 and closes #140 and #121
Changes:
Screencast.from.2024-03-20.15-07-31.webm