-
-
Notifications
You must be signed in to change notification settings - Fork 491
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
Fixes #2192 #1602 , Improving Filters UI #2245
Fixes #2192 #1602 , Improving Filters UI #2245
Conversation
Our Pull Request Approval ProcessWe have these basic policies to make the approval process smoother for our volunteer team. Testing Your CodePlease make sure your code passes all tests. Our test code coverage system will fail if either of these two conditions occur:
The process helps maintain the overall reliability of the code base and is a prerequisite for getting your PR approved. Assigned reviewers regularly review the PR queue and tend to focus on PRs that are passing. ReviewersWhen your PR has been assigned reviewers contact them to get your code reviewed and approved via:
Reviewing Your CodeYour reviewer(s) will have the following roles:
Other🎯 Please be considerate of our volunteers' time. Contacting the person who assigned the reviewers is not advised unless they ask for your input. Do not @ the person who did the assignment otherwise. |
@noman2002 @palisadoes What do you suggest for the count of lines. I think It requires this much code. |
Make components and reduce the number of lines in that particular file. |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #2245 +/- ##
===========================================
+ Coverage 88.27% 90.35% +2.08%
===========================================
Files 155 157 +2
Lines 7352 7507 +155
===========================================
+ Hits 6490 6783 +293
+ Misses 862 724 -138 ☔ View full report in Codecov by Sentry. |
@noman2002 Can you review please? |
lib/views/after_auth_screens/events/event_filter_bottomsheet.dart
Outdated
Show resolved
Hide resolved
lib/views/after_auth_screens/events/event_filter_bottomsheet.dart
Outdated
Show resolved
Hide resolved
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.
Add proper spacing/new-lines and improve documentation.
lib/services/navigation_service.dart
Outdated
@@ -118,6 +126,17 @@ class NavigationService { | |||
); | |||
} | |||
|
|||
/// This is used for the quick error of `duration: 2 seconds`. | |||
/// | |||
/// more_info_if_required |
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.
Remove this line. Edit the doc template accordingly.
@@ -232,6 +263,14 @@ class TalawaErrorWidget extends StatelessWidget { | |||
} | |||
} | |||
|
|||
///Tests Navigation Service. |
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.
Fix spacing and new line.
/// None | ||
/// | ||
/// **returns**: | ||
/// None |
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.
You don't need to write documentation for main function. Please read the doc guideline once.
pubspec.lock
Outdated
dart: ">=3.2.0-194.0.dev <3.13.0" | ||
flutter: ">=3.13.0" | ||
dart: ">=3.2.0 <3.13.0" | ||
flutter: ">=3.16.0" |
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.
Why is this bump in this PR ?
@literalEval made the changes you requested. Please Review. |
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.
- You don't need to write documentation for test files. Read the doc guideline thoroughly.
- Add proper spacing in documentation.
I'd suggest you to take smaller issues for now to get familiar with the codebase and documentation guidelines.
|
||
/// a_line_ending_with_end_punctuation. | ||
/// | ||
/// more_info_if_required |
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.
Fix this doc.
@literalEval can you point me to the doc where documentations guidelines are written? What I do is , I go with the custom lint and write documentation for each. also should I remove all the documentations in test files? |
@AyushRaghuvanshi here. Have a look at the official Dart documentation guidelines too. |
Understood. and have made changes according to that. @literalEval. |
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.
Use SizeConfig and not constant sizing.
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.
Add trailing commas at the end of long lines and reformat.
lib/views/after_auth_screens/events/event_filter_bottomsheet.dart
Outdated
Show resolved
Hide resolved
? Theme.of(context).colorScheme.secondary | ||
: AppTheme.white, | ||
borderRadius: const BorderRadius.all( | ||
Radius.circular(8), |
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.
Use dynamic sizing.
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.
8 here is radius.? do you want me to make this dynamic? I mean it would look weird to make it dependent upon the width or height. Also 8px radius would never cause any overflow error.
I can do it. But I dont think there is any need. Also in whole project we are using constant radius only.
@literalEval I have made the required changes. Can you review Please? |
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.
Looks fine.
@literalEval Please have a look.
@literalEval @palisadoes. I would like to work on More issues and this Pr is actually blocking both of my issues quota. Can you look into this? |
… Filters UI (PalisadoesFoundation#2245) * Add:Filters * Add:Documentation * Add:MultiLingual Support * Fix:Format * Reduced: Lines of Codes * Fixed:Tests * Add:Fix Failing test case * Add:Tests for requested lines * Add:test for filter changing * Fixed:changes requested * Add:Changes * Fix:Documentation * Fix:Dart version * Fixed:spacing * Remove: useless docs * Remove:Spaces * Removed:spaces * Fix:Constant Sizes * Fix:Format issues * Minor FIx * Fix:format * Fix:Dynamic size * Fix:Dynamic size * Fix:Failing test * Fix:Failing test
… Filters UI (PalisadoesFoundation#2245) * Add:Filters * Add:Documentation * Add:MultiLingual Support * Fix:Format * Reduced: Lines of Codes * Fixed:Tests * Add:Fix Failing test case * Add:Tests for requested lines * Add:test for filter changing * Fixed:changes requested * Add:Changes * Fix:Documentation * Fix:Dart version * Fixed:spacing * Remove: useless docs * Remove:Spaces * Removed:spaces * Fix:Constant Sizes * Fix:Format issues * Minor FIx * Fix:format * Fix:Dynamic size * Fix:Dynamic size * Fix:Failing test * Fix:Failing test
What kind of change does this PR introduce?
Feature : Improved the Filter UX and Filter by Date button.
Issue Number:
Fixes #2192 #1602
Did you add tests for your changes?
Its Just refactoring the UI. no tests required
Snapshots/Videos:
Does this PR introduce a breaking change?
No
Have you read the contributing guide?
Yes