-
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
Artefact page improvements #196
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.
Took me quite a while to go through all the changes, but I couldn't find anything that would break the main functionalities, as most of the changes are moving code around to optimize it. I really hope this will improve the review experience.
child: Consumer( | ||
builder: (context, ref, child) { | ||
final artefactBuilds = | ||
ref.watch(artefactBuildsProvider(artefact.id)); | ||
|
||
return artefactBuilds.when( | ||
loading: () => const YaruCircularProgressIndicator(), | ||
error: (e, stack) => | ||
Center(child: Text('Error:\n$e\nStackTrace:\n$stack')), | ||
data: (artefactBuilds) => const PageFiltersView( | ||
searchHint: 'Search by environment name', | ||
width: double.infinity, | ||
), | ||
); | ||
}, | ||
), |
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.
If I'm not wrong this code is rendering the side filter, do you think we can move it to a separate widget, perhaps a private one?
'Timestamp', | ||
style: TextStyle(fontStyle: FontStyle.italic), | ||
), | ||
), |
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 seems that here we have some formatting changes, should we rebase to main to use the formatter you added in the previous PR?
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.
There's no formatting changes here (already merged with main). But I've removed the SingleChildScrollView
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!
Description
A bunch of improvements to artefact page:
Resolved issues
Resolves https://warthogs.atlassian.net/browse/RTW-335
Documentation
None
Web service API changes
None
Tests
See video below for the overall experience:
Screencast from 2024-07-30 14-05-18.webm
Image below for test event tooltip: