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

[feature] mentions #706

Open
wants to merge 45 commits into
base: dev
Choose a base branch
from

Conversation

florian4f6c6
Copy link
Contributor

Types

  • Fix (non-breaking change which fixes an issue)
  • Feature (non-breaking change which adds functionality)
  • Refactor (non-breaking change which improves code quality - QA thoroughly )
  • Breaking Change (fix or feature that would cause existing functionality to not work as expected)
  • Chore (updates that would not affect or change the code involving the app itself)

Changes

Added a dialog that allows to select users of a room to mark them in the chat.

Attention! Known bug not all members of the room are currently loaded, unfortunately I can't find this bug. I have opened a pull request so that it is possible to review the code and possibly fix the bug.

@florian4f6c6 florian4f6c6 requested a review from ereio as a code owner July 2, 2022 15:37
Copy link
Collaborator

@EdGeraghty EdGeraghty left a comment

Choose a reason for hiding this comment

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

Your problem with the roomUsers is a problem with the room Model. I note this is due for a refactor...

image

lib/views/home/chat/widgets/chat-input.dart Outdated Show resolved Hide resolved
lib/views/home/chat/widgets/chat-input.dart Outdated Show resolved Hide resolved
@florian4f6c6
Copy link
Contributor Author

Your problem with the roomUsers is a problem with the room Model. I note this is due for a refactor...

image

Does that mean I can ignore it for now?

@EdGeraghty
Copy link
Collaborator

Does that mean I can ignore it for now?

Yes, I believe so.

@EdGeraghty
Copy link
Collaborator

You can take advantage of widget.controller.selection.baseOffset to find out where the blinking cursor currently is in the input. From that, you should be able to support mentions anywhere in the text box :)

@EdGeraghty EdGeraghty self-assigned this Aug 24, 2022
lib/views/home/chat/widgets/chat-input.dart Outdated Show resolved Hide resolved
lib/views/home/chat/widgets/chat-input.dart Outdated Show resolved Hide resolved
final bool loading = widget.sending;
final double maxInputHeight = replying ? height * 0.45 : height * 0.65;
final double maxMediaHeight = keyboardHeight > 0 ? keyboardHeight : height * 0.38;
final double maxInputHeight =
Copy link
Member

Choose a reason for hiding this comment

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

make sure your default column width is set to 110, it's the default for Syphon

: 0),
topRight: Radius.circular(!replying
? DEFAULT_BORDER_RADIUS
: 0),
Copy link
Member

Choose a reason for hiding this comment

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

lots of these updates are just column reduction, I'll review more once you've set the default to 110

I'll try to find time these weekend to find a way to make this work by default for whoever forks the project

It used to be very difficult to enforce similar to eslint, where it just requires the config to be local

lib/views/home/chat/widgets/chat-mention.dart Outdated Show resolved Hide resolved
@ereio
Copy link
Member

ereio commented Aug 25, 2022

Most of the requests I made above are styling changes. Happy to help make these - and again - find a way to automate some of this where you have no choice but to follow the convention. Dart / Flutter used to do a poor job at enforcement, where as other languages allow much more tooling around linting that makes it easier to just pull and auto format

I'll take a look this week

@ereio ereio added the ready for review ready for review or merging label Aug 25, 2022
Copy link
Collaborator

@EdGeraghty EdGeraghty left a comment

Choose a reason for hiding this comment

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

So very close, now :)

lib/views/home/chat/widgets/chat-input.dart Outdated Show resolved Hide resolved
lib/views/home/chat/widgets/chat-input.dart Outdated Show resolved Hide resolved
@EdGeraghty
Copy link
Collaborator

I'm unable to test this

image

@EdGeraghty
Copy link
Collaborator

image

I invalidated all of my caches and built from scratch just to make sure

@florian4f6c6
Copy link
Contributor Author

Perhaps something wrong with your settings

@ereio ereio changed the title [Feature] mentions [feature] mentions Sep 11, 2022
@ereio
Copy link
Member

ereio commented Sep 11, 2022

I will pull this branch and clean it up before reviewing @Florian-Sabonchi. No need to make changes just yet

Aiming to have the 0.2.14 branch out next weekend and test the dev branch extensively throughout the week

@ereio
Copy link
Member

ereio commented Dec 4, 2022

@Florian-Sabonchi pushed a branch here targeting your PR! I've refactored what's needed and will approve this PR after the merge. Great work! :)

florian4f6c6#4

refactor: cleaned up mentions branch
@florian4f6c6
Copy link
Contributor Author

@Florian-Sabonchi pushed a branch here targeting your PR! I've refactored what's needed and will approve this PR after the merge. Great work! :)

Florian-Sabonchi#4

Hi, Thanks for your work. I merged your pull request seems that we have some merge Konflikts because long time ago i made the initial Pull Request

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready for review ready for review or merging
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants