Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

#

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Jump to bottom

Made message bar expand up to 4 rows #586

Merged
merged 1 commit into from
Nov 29, 2021
Merged

Made message bar expand up to 4 rows #586

merged 1 commit into from
Nov 29, 2021

Conversation

oxtoacart
Copy link

  • All strings are defined using localized message keys like 'my_message_key'.i18n and are defined in en.po.
  • All colors are defined using Hex (not using built-in colors), are defined in colors.dart and are not duplicated
  • All text styles are defined in text_styles.dart and are not duplicated
  • All icons are using the vector resource from Figma (do not use built-in Icons)
  • Repeated code has been factored into custom widgets
  • Layout looks good in both LTR (English) and RTL (Persian) languages
  • If you refactored existing code, have you tested the refactored functionality against the old version to make sure you didn't break anything?
  • Do the tests pass? Consistently?
  • Did this change improve test coverage?
  • Is the code in question being linted? If not, consider adding a linter step to CI. If yes, make sure the linter is happy.
  • Have you logged tickets for related technical debt with the label “techdebt”?

cosmicespresso
cosmicespresso reviewed Nov 29, 2021
View reviewed changes
lib/messaging/conversation/conversation.dart
),
style: tsSubtitle1
.copiedWith(color: isSendIconVisible ? black : grey5)
.short,
Copy link

Choose a reason for hiding this comment

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

This makes it look a little bit cramped vertically? I would maybe create a new line height between short and our default.

Copy link
Author

Choose a reason for hiding this comment

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

Good catch. We'll have to make sure that alignment in the single-line case still looks correct.

cosmicespresso
cosmicespresso reviewed Nov 29, 2021
View reviewed changes
lib/messaging/conversation/conversation.dart
maxLines: 4,
autofocus: false,
textInputAction: TextInputAction.send,
controller: newMessage,
Copy link

Choose a reason for hiding this comment

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

Weird edge case:

  • write a bunch of stuff (native text keyboard)
  • the message bar correctly scrolls
  • use emoji keyboard to input emojis
  • the message bar doesn't scroll to the newest emoji unless we switch to text keyboard

Copy link
Author

Choose a reason for hiding this comment

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

Ah, interesting catch! Can you look into this please?

cosmicespresso
cosmicespresso reviewed Nov 29, 2021
View reviewed changes
Copy link

@cosmicespresso cosmicespresso left a comment

Choose a reason for hiding this comment

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

Two comments:

  • I think we should increase the line height a bit
  • Edge case with scrolling and switching between emoji <--> native keyboard

@cosmicespresso cosmicespresso merged commit 50291a0 into kr/expandMessageBar Nov 29, 2021
@cosmicespresso cosmicespresso deleted the ox/expandMessageBar branch November 29, 2021 15:52
# for free to join this conversation on GitHub. Already have an account? # to comment
Reviewers

@cosmicespresso cosmicespresso cosmicespresso left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@oxtoacart @cosmicespresso