Skip to content

chat adaptive layout and general improvements#881

Merged
gtalha07 merged 6 commits intoacterglobal:mainfrom
gtalha07:issue844_chat_adaptive-layout
Aug 27, 2023
Merged

chat adaptive layout and general improvements#881
gtalha07 merged 6 commits intoacterglobal:mainfrom
gtalha07:issue844_chat_adaptive-layout

Conversation

@gtalha07
Copy link
Contributor

@gtalha07 gtalha07 commented Aug 23, 2023

@gtalha07
Copy link
Contributor Author

Normal Split View:

Screenshot 2023-08-24 at 4 47 42 AM

Smaller Split View:

Screenshot 2023-08-24 at 4 47 58 AM

Full Split View:

Screenshot 2023-08-24 at 4 48 18 AM

@gtalha07 gtalha07 marked this pull request as ready for review August 24, 2023 16:59
final convo = notification.convo()!;
avatar = Consumer(
builder: (ctx, ref, child) {
builder: (context, ref, child) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually I renamed inner context to ctx so that it can be distinguished with outer context.

@gnunicorn
Copy link
Contributor

tested. love it!

@gnunicorn
Copy link
Contributor

gnunicorn commented Aug 25, 2023

Found one problem when routing to the dashboard though:

flutter:  -------      refreshing news
[2023-08-25][09:32:41][acter::platform::native][ERROR] dependOnInheritedWidgetOfExactType<UncontrolledProviderScope>() or dependOnInheritedElement() was called before _DashboardState.initState() completed.
When an inherited widget changes, for example if the value of Theme.of() changes, its dependent widgets are rebuilt. If the dependent widget's reference to the inherited widget is in a constructor or an initState() method, then the rebuilt dependent widget will not reflect the changes in the inherited widget.
Typically references to inherited widgets should occur in widget build() methods. Alternatively, initialization based on inherited widgets can be placed in the didChangeDependencies method, which is called after initState and whenever the dependencies change thereafter.
[2023-08-25][09:32:41][acter::platform::native][ERROR] #0      StatefulElement.dependOnInheritedElement.<anonymous closure> (package:flutter/src/widgets/framework.dart:5321:9)
#1      StatefulElement.dependOnInheritedElement (package:flutter/src/widgets/framework.dart:5364:6)
#2      Element.dependOnInheritedWidgetOfExactType (package:flutter/src/widgets/framework.dart:4383:14)
#3      ProviderScope.containerOf (package:flutter_riverpod/src/framework.dart:99:12)
#4      ConsumerStatefulElement._container (package:flutter_riverpod/src/consumer.dart:511:53)
#5      ConsumerStatefulElement._container (package:flutter_riverpod/src/consumer.dart)
#6      ConsumerStatefulElement.watch.<anonymous closure> (package:flutter_riverpod/src/consumer.dart:560:14)
...

Copy link
Contributor

@gnunicorn gnunicorn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice. just a glance overall, as we agreed to do the proper routing later. but still a few remarks, I'd like to have addressed before merging...

Routes.chatroom.name,
pathParameters: {'roomId': roomId},
);
if (!isDesktop(context)) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not fond of this, but good enough for now. let's work on the actual routing in another PR

? const RoomPage()
: const SizedBox(
child: Center(
child: Text('Tap on any room to see full preview'),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

preview?

Suggested change
child: Text('Tap on any room to see full preview'),
child: Text('Select any room to view it'),

but maybe we also want the designers to come up with something better...

final convo = ref.watch(currentConvoProvider);
return LayoutBuilder(
builder: (context, constraints) {
return Row(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

did you ever test this on a completely fresh empty account without any chats yet? what does that look like?

Copy link
Contributor Author

@gtalha07 gtalha07 Aug 25, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No. I think it should reflect the empty placeholder for overview. But for RoomPage(), we might want to show empty or just hide it.

child: SettingsMenu(),
),
Expanded(child: child),
const Flexible(flex: 1, child: SettingsMenu()),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

are we actually certain, this works on width == 771? Because 771/3 = 257, which is 100 smaller than the 350 it had before ...

flutter_mentions: ^2.0.1
flutter_mentions:
git:
url: https://github.com/gtalha07/flutter_mentions.git
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what is happening here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Currently, flutter_mentions highly requested feature was to include suggestionListWidth so we can change the width of suggestion list container. I did that in my forked version as its needed for making it adaptive across different screen sizes. I am looking to open contribution PR for it but also seeing the repository being not actively maintained .i.e. last was 2 yrs ago...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems like the PR is already opened for review and merge, but no updates on it yet...

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks for explanation. If we need to maintain a fork, we probably want to move it into our org though ...

@gtalha07
Copy link
Contributor Author

gtalha07 commented Aug 25, 2023

Found one problem when routing to the dashboard though:

flutter:  -------      refreshing news
[2023-08-25][09:32:41][acter::platform::native][ERROR] dependOnInheritedWidgetOfExactType<UncontrolledProviderScope>() or dependOnInheritedElement() was called before _DashboardState.initState() completed.
When an inherited widget changes, for example if the value of Theme.of() changes, its dependent widgets are rebuilt. If the dependent widget's reference to the inherited widget is in a constructor or an initState() method, then the rebuilt dependent widget will not reflect the changes in the inherited widget.
Typically references to inherited widgets should occur in widget build() methods. Alternatively, initialization based on inherited widgets can be placed in the didChangeDependencies method, which is called after initState and whenever the dependencies change thereafter.
[2023-08-25][09:32:41][acter::platform::native][ERROR] #0      StatefulElement.dependOnInheritedElement.<anonymous closure> (package:flutter/src/widgets/framework.dart:5321:9)
#1      StatefulElement.dependOnInheritedElement (package:flutter/src/widgets/framework.dart:5364:6)
#2      Element.dependOnInheritedWidgetOfExactType (package:flutter/src/widgets/framework.dart:4383:14)
#3      ProviderScope.containerOf (package:flutter_riverpod/src/framework.dart:99:12)
#4      ConsumerStatefulElement._container (package:flutter_riverpod/src/consumer.dart:511:53)
#5      ConsumerStatefulElement._container (package:flutter_riverpod/src/consumer.dart)
#6      ConsumerStatefulElement.watch.<anonymous closure> (package:flutter_riverpod/src/consumer.dart:560:14)
...

I can't seem to reproduce this on my side. Been navigating in between on debug but no errors. Could you provide more detail about the issue .i.e. platform, user account etc. (Not sure if this issue is related with PR either)

@gnunicorn
Copy link
Contributor

@gtalha07 well, then ignore this issue for now. if it comes back up, we'll deal with it then.

@gtalha07 gtalha07 merged commit 6a79f3d into acterglobal:main Aug 27, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.

Chat overviewlist when we have enough side-space

3 participants