-
Notifications
You must be signed in to change notification settings - Fork 250
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
feat: mobile support #227
feat: mobile support #227
Conversation
Currently, I have tried to make the most of existing widgets by adding |
@DenserMeerkat Thanks for the PR. We will review it and get back. |
@DenserMeerkat You are going in the right direction. I recommend you to continue thinking about the end user and improve the UI/UX to make his life easier and the app more intuitive to use. Some feedback points:
227.header.rangeerror.mov
|
@animator Thanks for taking the time to review my implementation! I appreciate you pointing out the scrolling and focus problems in the widgets. I too noticed these and I'm already working on solutions to ensure these widgets work flawlessly with touch interactions and deliver the intended functionality. Response IconI spent some time trying to find an icon which represents API Response but couldn't find a particular one so settled with the Comment icon. Will try finding alternatives. View Code buttonInitially just had the "< >" icon in button for a clean layout and clear button hierarchy, but added "View Code" label for user's clarity of the button's function. Happy to revert based on your feedback. Will get back to you if I have any updates/doubts on fixes. |
@animator |
Yes, all three should be there. |
lib/providers/ui_providers.dart
Outdated
|
||
final mobileDrawerKeyProvider = Provider<GlobalKey<InnerDrawerState>>( |
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 StateProvider
@@ -143,14 +142,21 @@ class _RequestListState extends ConsumerState<RequestList> { | |||
final alwaysShowCollectionPaneScrollbar = ref.watch(settingsProvider | |||
.select((value) => value.alwaysShowCollectionPaneScrollbar)); | |||
final filterQuery = ref.watch(searchQueryProvider).trim(); | |||
final isMobile = | |||
kIsMobile && MediaQuery.sizeOf(context).width < kMinWindowSize.width; |
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.
Is it possible to create an extension over context like context.isMobile
context.isMobile = kIsMobile && MediaQuery.sizeOf(context).width < kMinWindowSize.width
as this code will be used in a lot of places
lib/screens/mobile/left_drawer.dart
Outdated
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.
Create a widgets
folder inside mobile
for mobile related widgets. All Stateful & Stateless widgets will go there.
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.
What is the purpose of this folder requests_page
?
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.
In desktop version there is no such folder.
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.
Initially wanted to segregate widgets into
- Request page widgets :
- request list (left menu)
- reqquest editor
- response viewer
- Environment page widgets
- environment list (left menu)
- environment editor
but as you mentioned since we are gonna have a separate folder for environments, yes, it doesn't make sense to have it inside the requests_page
folder
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 ResponseDrawer
inside requests_page
?
lib/utils/http_utils.dart
Outdated
@@ -7,14 +7,14 @@ import '../models/models.dart'; | |||
import 'convert_utils.dart' show rowsToMap; | |||
import '../consts.dart'; | |||
|
|||
String getRequestTitleFromUrl(String? url) { | |||
String getRequestTitleFromUrl(String? url, {bool capitalize = false}) { | |||
if (url == null || url.trim() == "") { |
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 this unnecessary change?
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.
Felt the lowercase 'untitled' Appbar title didn't look nice, but as you said, yes, it was an unnecessary change will remove it.
lib/widgets/dropdowns.dart
Outdated
child: Text( | ||
value.name.toUpperCase(), | ||
style: kCodeStyle.copyWith( | ||
fontSize: isMobile ? 13 : null, |
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 font size is being hard coded here just for dropdown .. it should properly flow from consts.dart?
lib/widgets/intro_message.dart
Outdated
@@ -23,6 +23,8 @@ class IntroMessage extends StatelessWidget { | |||
return FutureBuilder( | |||
future: introData(), | |||
builder: (BuildContext context, AsyncSnapshot<void> snapshot) { | |||
final isMobile = kIsMobile && | |||
MediaQuery.sizeOf(context).width < kMinWindowSize.width; |
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.
again .. needs to be shortened everywhere by defining an extension over context.
lib/app.dart
Outdated
@@ -105,6 +105,8 @@ class DashApp extends ConsumerWidget { | |||
Widget build(BuildContext context, WidgetRef ref) { | |||
final isDarkMode = | |||
ref.watch(settingsProvider.select((value) => value.isDark)); | |||
final isLargeMobile = | |||
MediaQuery.sizeOf(context).width > kMinWindowSize.width; |
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.
can be moved to new context_extensions.dart
@@ -4,6 +4,7 @@ import 'package:apidash/providers/providers.dart'; | |||
import 'package:apidash/widgets/widgets.dart'; | |||
import 'package:apidash/models/models.dart'; | |||
import 'package:apidash/consts.dart'; | |||
import 'package:apidash/extensions/extensions.dart' show MediaQueryExtension; |
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 can directly import 'package:apidash/extensions/extensions.dart' everywhere, no need to show MediaQueryExtension.
@@ -4,22 +4,44 @@ import 'package:apidash/providers/providers.dart'; | |||
import 'package:apidash/consts.dart'; | |||
import 'details_card/details_card.dart'; | |||
import 'url_card.dart'; | |||
import 'package:apidash/screens/home_page/editor_pane/details_card/request_pane/request_pane.dart'; |
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.
This import needs cleaning up
PR Description
Implementing the Figma Link design with maximum (almost all) available components, to discuss the design decisions.
demo.mp4
Related Issues
Checklist
flutter test
) and all tests are passingAdded/updated tests?