-
Notifications
You must be signed in to change notification settings - Fork 1
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
fix: layout issues on android #804
Conversation
Reviewer's Guide by SourceryThis pull request addresses layout issues on Android by introducing conditional styling based on whether the application is running on a desktop or mobile device. It leverages the Updated class diagram for UI components with conditional stylingclassDiagram
class App {
+layout_class: Signal<String>
}
class Browse {
+layout_class: Signal<String>
+input_class: Signal<String>
+grid_class: Signal<String>
}
class ResolvedServiceItem {
+card_class: Signal<String>
}
class AutoCompleteServiceType {
+input_class: Signal<String>
+class: Signal<String>
}
class Metrics {
}
note for App "App component now uses layout_class Signal for conditional styling."
note for Browse "Browse component now uses layout_class, input_class and grid_class Signals for conditional styling."
note for ResolvedServiceItem "ResolvedServiceItem component now uses card_class Signal for conditional styling."
note for AutoCompleteServiceType "AutoCompleteServiceType component now uses input_class and class Signals for conditional styling."
File-Level Changes
Assessment against 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 @hrzlgnm - I've reviewed your changes - here's some feedback:
Overall Comments:
- Consider using a single
Signal::derive
to determine the class name instead of multiple signals. - It looks like you're querying the
IsDesktopSignal
context in multiple components; consider creating a custom hook to encapsulate this logic.
Here's what I looked at during the review
- 🟡 General issues: 1 issue found
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟡 Complexity: 1 issue found
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Closes #792
Summary by Sourcery
Enhancements: