-
Notifications
You must be signed in to change notification settings - Fork 814
feat: added layout for the Wave Generator screen #2803
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
Conversation
Reviewer's GuideIntroduces a Wave Generator instrument screen by registering its route, extending localizations, and adding stub widgets for the UI layout including controls and graph rendering. File-Level Changes
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Build successful. APKs to test: https://github.com/fossasia/pslab-app/actions/runs/16851390869/artifacts/3726365585 |
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 @AsCress - I've reviewed your changes - here's some feedback:
- You have a lot of repeated button‐styling code in AnalogWaveformControls and DigitalWaveformControls—consider extracting a reusable widget or generating the buttons from a configuration list to reduce duplication.
- There are several hard-coded English strings (e.g. ‘Wave Generator’, ‘Sine’, ‘Frequency’, ‘Phase Offset’) that should be pulled into your localization ARB files and referenced via AppLocalizations.
- Instead of grabbing AppLocalizations from getIt in your state class, use AppLocalizations.of(context) in build() so the UI properly updates when the app locale changes.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- You have a lot of repeated button‐styling code in AnalogWaveformControls and DigitalWaveformControls—consider extracting a reusable widget or generating the buttons from a configuration list to reduce duplication.
- There are several hard-coded English strings (e.g. ‘Wave Generator’, ‘Sine’, ‘Frequency’, ‘Phase Offset’) that should be pulled into your localization ARB files and referenced via AppLocalizations.
- Instead of grabbing AppLocalizations from getIt in your state class, use AppLocalizations.of(context) in build() so the UI properly updates when the app locale changes.
## Individual Comments
### Comment 1
<location> `lib/view/widgets/wave_generator_main_controls.dart:125` </location>
<code_context>
+ width: 30,
+ child: IconButton.filled(
+ padding: EdgeInsets.zero,
+ icon: Icon(Icons.chevron_left),
+ onPressed: () async {},
+ style: IconButton.styleFrom(
+ backgroundColor: primaryRed,
</code_context>
<issue_to_address>
Empty onPressed for navigation buttons may confuse users.
If these buttons are placeholders, disable them or add a TODO to clarify their purpose.
</issue_to_address>
<suggested_fix>
<<<<<<< SEARCH
child: IconButton.filled(
padding: EdgeInsets.zero,
icon: Icon(Icons.chevron_left),
onPressed: () async {},
style: IconButton.styleFrom(
backgroundColor: primaryRed,
shape: RoundedRectangleBorder(
borderRadius: BorderRadius.circular(4),
),
),
),
=======
// TODO: Implement navigation logic for chevron_left button
child: IconButton.filled(
padding: EdgeInsets.zero,
icon: Icon(Icons.chevron_left),
onPressed: null, // Disabled until implemented
style: IconButton.styleFrom(
backgroundColor: primaryRed,
shape: RoundedRectangleBorder(
borderRadius: BorderRadius.circular(4),
),
),
),
>>>>>>> REPLACE
</suggested_fix>
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
@CloudyPadmal @marcnause We can merge this without worrying about the hardcoding again. The changes associated with the Wave Generator instrument are quite large so I'm pushing in parts. This PR would be followed up by another one which would add all the functionality. |
Adds the layout for the Wave Generator instrument screen.
Screenshots
Summary by Sourcery
Add Wave Generator instrument screen with route registration and navigation support
New Features:
Summary by Sourcery
Add the Wave Generator instrument screen layout with navigation integration and localization support
New Features: