Skip to content
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

Chat input redesigned #2478

Merged
merged 8 commits into from
Jan 10, 2025
Merged

Conversation

gtalha07
Copy link
Contributor

@gtalha07 gtalha07 commented Jan 6, 2025

Description:

Redesigned chat input with placeholder .i.e. hint text showing on empty state. (Encrypted / Non-encrypted message).

NOTE: the placeholder seems to not disappear when entering newline (if the first line is empty). This is problematic on appflowy end considering it somehow restricts showing placeholder on every empty line. And even modifying the conditions where we can make it disappear but throws internal exceptions. However, this can't be regard as much important because it disappears as soon something is typed.

(Desktop):

Screen.Recording.2025-01-06.at.19.17.21.mov

(Mobile):

ScreenRecording_01-06-2025.19-21-36_1.MP4

Also fixes #2469 / #2456 with reference in change-set and with proof:

  • Example where previously enclosing in code block was making it grey and now its disappeared:
ScreenRecording_01-06-2025.19-29-19_1.MP4

Copy link

codecov bot commented Jan 6, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 28.24%. Comparing base (b9826d9) to head (4939548).
Report is 52 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2478      +/-   ##
==========================================
+ Coverage   28.01%   28.24%   +0.22%     
==========================================
  Files         660      680      +20     
  Lines       44775    45715     +940     
==========================================
+ Hits        12542    12910     +368     
- Misses      32233    32805     +572     
Flag Coverage Δ
integration-test 39.26% <ø> (+0.93%) ⬆️
unittest 18.97% <ø> (-0.27%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

final map = {...standardBlockComponentBuilderMap};
map[ParagraphBlockKeys.type] = ParagraphBlockComponentBuilder(
// only show placeholder at first line
showPlaceholder: (editorState, node) {
Copy link
Contributor Author

@gtalha07 gtalha07 Jan 6, 2025

Choose a reason for hiding this comment

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

This is actual placeholder showing logic, but modifying it to make it disappear upon entering newline (without any content on first line) seems to be not allowed and throws exception.

final mention = attributes.entries
.firstWhere((e) => e.value is MentionAttributes)
.value as MentionAttributes?;
MentionAttributes? mention;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This fix solves the gray area / bad state: exception problem because of nullable object here and now we have wrapped with proper error handling and return fallback in case, it fails.

return Expanded(
child: IntrinsicHeight(
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This IntrinsicHeight wrapper isn't needed as opposed to inner one, so having it removed is recommended as it's expensive.

Copy link
Contributor

@gnunicorn gnunicorn Jan 9, 2025

Choose a reason for hiding this comment

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

@gtalha07 gtalha07 marked this pull request as ready for review January 6, 2025 14:47
@gtalha07 gtalha07 linked an issue Jan 6, 2025 that may be closed by this pull request
.changes/2478-chat-input-redesigned.md Outdated Show resolved Hide resolved
app/lib/common/widgets/html_editor/html_editor.dart Outdated Show resolved Hide resolved
Comment on lines 223 to 241
// safety check
if (node.path.isEmpty) return false;

// Check if entire document is empty
final doc = editorState.document;
final isEmpty = doc.root.children.every(
(node) => node.delta?.isEmpty ?? true,
);

// Show placeholder only on first line when document is empty
return isEmpty && node.path[0] == 0;
},
// (node.delta?.isEmpty ?? true),
configuration: BlockComponentConfiguration(
placeholderText: (node) => widget.hintText ?? ' ',
),
);

return map;
Copy link
Contributor

Choose a reason for hiding this comment

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

Don't get much understanding here, would like to request @gnunicorn to review this.

@kumarpalsinh25
Copy link
Contributor

@gtalha07

Screenshot 2025-01-08 at 10 57 20 AM

Out of this PR scope, from reference video I found that this chat bubbles looks pretty weird.

@gtalha07
Copy link
Contributor Author

gtalha07 commented Jan 8, 2025

@gtalha07

Screenshot 2025-01-08 at 10 57 20 AM

Out of this PR scope, from reference video I found that this chat bubbles looks pretty weird.

I guess it's the format. These inputs are solely used for testing. But that's not the common pattern of conversation in daily use. So I didn't considered much here. Unless you have some proposition? :)

@gnunicorn
Copy link
Contributor

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.

seems fine

.changes/2478-chat-input-redesigned.md Outdated Show resolved Hide resolved
app/lib/common/widgets/html_editor/html_editor.dart Outdated Show resolved Hide resolved
app/lib/common/widgets/html_editor/html_editor.dart Outdated Show resolved Hide resolved
Comment on lines +162 to 169
color: Theme.of(context).colorScheme.primaryContainer,
borderRadius: BorderRadius.only(
topLeft: Radius.circular(15.0),
topRight: Radius.circular(15.0),
),
child: Row(
children: [
leadingBtn(emojiPickerVisible),
editorField(),
trailingBtn(),
],
border: BorderDirectional(
top: BorderSide(color: greyColor),
),
Copy link
Contributor

Choose a reason for hiding this comment

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

I am not 100% sure about this new outline design change. It seems to be pushing the UI element to the front/above the other chat elements and thus giving the impression of being attached to the bottom bar rather than being part of the chat.

But let's not stop on this for now, we can revisit that once we are thinking of switching the labs to default on again.

return Expanded(
child: IntrinsicHeight(
Copy link
Contributor

@gnunicorn gnunicorn Jan 9, 2025

Choose a reason for hiding this comment

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

@gtalha07 gtalha07 enabled auto-merge January 10, 2025 11:40
@gtalha07 gtalha07 merged commit 7743002 into acterglobal:main Jan 10, 2025
23 of 24 checks passed
@gtalha07 gtalha07 deleted the chat-input-redesigned branch January 14, 2025 15:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Recently Done
3 participants