-
Notifications
You must be signed in to change notification settings - Fork 815
fix: added snackbar and small ui changes in lux meter. #2747
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
base: flutter
Are you sure you want to change the base?
Conversation
Reviewer's GuideIntroduces user feedback for light sensor errors via Snackbar, enhances sensor lifecycle and platform-specific error handling, and refines the lux meter chart’s UI styling and responsiveness. Sequence diagram for light sensor error handling and Snackbar feedbacksequenceDiagram
actor User
participant LuxMeterScreen
participant LuxMeterStateProvider
participant Snackbar
User->>LuxMeterScreen: Open screen
LuxMeterScreen->>LuxMeterStateProvider: initializeSensors(onError)
alt Platform not supported or sensor error
LuxMeterStateProvider-->>LuxMeterScreen: onError callback with error message
LuxMeterScreen->>Snackbar: Show error message
else Sensor available
LuxMeterStateProvider-->>LuxMeterScreen: Sensor data updates
LuxMeterScreen->>User: Display chart
end
File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
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.
Hey @Yugesh-Kumar-S - I've reviewed your changes - here's some feedback:
- The direct import of
dart:io
inluxmeter_state_provider.dart
will break web builds—use conditional imports or platform-agnostic abstractions to avoid build errors on non-native targets. - There’s a lot of duplicated logic across
_handleUnsupportedPlatform
,_handleSensorError
, and_handleSensorNotAvailable
—consider unifying these into a single streamlined error‐handling flow to reduce repetition. - The chart layout code repeats padding and reserved size calculations for different breakpoints—extract those values into helper constants or methods to improve readability and maintainability.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The direct import of `dart:io` in `luxmeter_state_provider.dart` will break web builds—use conditional imports or platform-agnostic abstractions to avoid build errors on non-native targets.
- There’s a lot of duplicated logic across `_handleUnsupportedPlatform`, `_handleSensorError`, and `_handleSensorNotAvailable`—consider unifying these into a single streamlined error‐handling flow to reduce repetition.
- The chart layout code repeats padding and reserved size calculations for different breakpoints—extract those values into helper constants or methods to improve readability and maintainability.
## Individual Comments
### Comment 1
<location> `lib/view/luxmeter_screen.dart:146` </location>
<code_context>
- timeAxisLabel,
+ final reservedSizeLeft = screenWidth < 400 ? 27.0 : 30.0;
+ final reservedSizeRight = screenWidth < 400 ? 27.0 : 30.0;
+ return Padding(
+ padding: const EdgeInsets.only(right: 20.0),
+ child: LineChart(
+ LineChartData(
+ backgroundColor: Colors.black,
</code_context>
<issue_to_address>
Adding right padding to the chart may cause misalignment with other UI elements.
Consider using a responsive value or matching the card's padding to ensure consistent alignment across different screen sizes.
Suggested implementation:
```
// Define a responsive horizontal padding, or use the card's padding value if available
final horizontalPadding = screenWidth < 400 ? 12.0 : 20.0;
return Padding(
padding: EdgeInsets.only(right: horizontalPadding),
```
If the card's padding is defined elsewhere (e.g., as a constant or theme value), you should reference that value instead of defining `horizontalPadding` here. Also, consider applying the same `horizontalPadding` to the left side if you want symmetrical padding for better alignment.
</issue_to_address>
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
return Padding( | ||
padding: const EdgeInsets.only(right: 20.0), | ||
child: LineChart( |
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.
suggestion: Adding right padding to the chart may cause misalignment with other UI elements.
Consider using a responsive value or matching the card's padding to ensure consistent alignment across different screen sizes.
Suggested implementation:
// Define a responsive horizontal padding, or use the card's padding value if available
final horizontalPadding = screenWidth < 400 ? 12.0 : 20.0;
return Padding(
padding: EdgeInsets.only(right: horizontalPadding),
If the card's padding is defined elsewhere (e.g., as a constant or theme value), you should reference that value instead of defining horizontalPadding
here. Also, consider applying the same horizontalPadding
to the left side if you want symmetrical padding for better alignment.
Build successful. APKs to test: https://github.com/fossasia/pslab-android/actions/runs/15740427591/artifacts/3357133187 |
Co-authored-by: Padmal <[email protected]>
Fixes #2746
Changes
Screenshots / Recordings
Checklist:
strings.xml
,dimens.xml
andcolors.xml
without hard coding any value.strings.xml
,dimens.xml
orcolors.xml
.Summary by Sourcery
Add snackbar notifications for light sensor errors, refactor sensor initialization and disposal, and adjust the lux meter chart’s UI styling and layout.
New Features:
Enhancements: