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

Crdzbird/conversation messaging scaffold hi fi #354

Merged

Conversation

Crdzbird
Copy link

@Crdzbird Crdzbird commented Sep 3, 2021

This PR is to setup the Scaffold construction for the Messaging UI.

I have to update the base_screen.dart to have more flexibility with the components that are gonna be to be displayed.

  • 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”?

- Removed static title to have a more flexible widget title, this was needed to have a clean approach to the HiFi design on Figma
@Crdzbird Crdzbird requested a review from oxtoacart September 3, 2021 20:17
@@ -26,7 +26,10 @@ class _IntroduceState extends State<Introduce> {
Widget build(BuildContext context) {
var model = context.watch<MessagingModel>();
return BaseScreen(
title: 'Introduce Contacts (${selectedContactIds.length})'.i18n,
Copy link
Author

@Crdzbird Crdzbird Sep 3, 2021

Choose a reason for hiding this comment

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

Almost all of the other files just have one change and that is on the title param which now requires a Widget instead of a String

title: 'Messages'.i18n,
title: Text(
'Messages'.i18n,
style: tsTitleAppbar,

Choose a reason for hiding this comment

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

I don't love that this repeats. Maybe BaseScreen can have two parameters, String titleText and Widget title. Then we can just have the styling for the titleText inside of BaseScreen. What do you think @Crdzbird ?

Copy link
Author

Choose a reason for hiding this comment

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

Sure, we can have a dynamic param instead, and just based on the type we can set a default widget.

@Crdzbird Crdzbird requested a review from oxtoacart September 3, 2021 20:32
title: title,
logoTitle: logoTitle,
appBar: AppBar(
title: title.runtimeType == String
Copy link
Author

Choose a reason for hiding this comment

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

Instead of 2 params, we can just use one dynamic param and based on his type set a default widget.

Copy link
Author

Choose a reason for hiding this comment

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

If the second param is an absolute must, I can add it instead of the dynamic @oxtoacart

Copy link

@oxtoacart oxtoacart left a comment

Choose a reason for hiding this comment

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

LGTM

@Crdzbird
Copy link
Author

Crdzbird commented Sep 4, 2021

Thanks @oxtoacart!

@Crdzbird Crdzbird merged commit c563f18 into ox/messaging Sep 4, 2021
@Crdzbird Crdzbird deleted the crdzbird/conversation_messaging_scaffold_HiFi branch September 4, 2021 20:00
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants