-
Notifications
You must be signed in to change notification settings - Fork 13
Standardised textfield size #454
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
Changes from all commits
13e1283
7d7d57c
24c1376
d177ccf
80027a0
5d34e4e
257233f
a2f6813
85f4c10
081e9a3
a56e0a8
f7633af
4f91d50
65345f7
b520544
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||
|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -3,6 +3,8 @@ import 'dart:math'; | |||||||||
| import 'package:cached_network_image/cached_network_image.dart'; | ||||||||||
| import 'package:flutter/material.dart'; | ||||||||||
| import 'package:flutter_screenutil/flutter_screenutil.dart'; | ||||||||||
| import 'package:flutter_svg/svg.dart'; | ||||||||||
| import 'package:whitenoise/ui/core/themes/assets.dart'; | ||||||||||
|
Comment on lines
+6
to
+7
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Use the canonical flutter_svg import to avoid build errors This file imports Apply this diff: -import 'package:flutter_svg/svg.dart';
+import 'package:flutter_svg/flutter_svg.dart';📝 Committable suggestion
Suggested change
🤖 Prompt for AI Agents |
||||||||||
| import 'package:whitenoise/ui/core/themes/src/extensions.dart'; | ||||||||||
|
|
||||||||||
| class StackedImages extends StatelessWidget { | ||||||||||
|
|
@@ -119,7 +121,15 @@ class StackedImages extends StatelessWidget { | |||||||||
| color: Colors.grey[300], | ||||||||||
| width: size, | ||||||||||
| height: size, | ||||||||||
| child: Icon(Icons.error, size: 24.sp), | ||||||||||
| child: SvgPicture.asset( | ||||||||||
| AssetsPaths.icErrorFilled, | ||||||||||
| colorFilter: ColorFilter.mode( | ||||||||||
| context.colors.destructive, | ||||||||||
| BlendMode.srcIn, | ||||||||||
| ), | ||||||||||
| width: 24.w, | ||||||||||
| height: 24.w, | ||||||||||
| ), | ||||||||||
| ), | ||||||||||
| ) | ||||||||||
| : Image.file( | ||||||||||
|
|
||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -11,7 +11,9 @@ import 'package:whitenoise/config/providers/contacts_provider.dart'; | |
| import 'package:whitenoise/config/providers/group_provider.dart'; | ||
| import 'package:whitenoise/domain/models/contact_model.dart'; | ||
| import 'package:whitenoise/domain/services/key_package_service.dart'; | ||
| import 'package:whitenoise/src/rust/api.dart'; | ||
| import 'package:whitenoise/src/rust/api/groups.dart'; | ||
| import 'package:whitenoise/src/rust/api/utils.dart'; | ||
| import 'package:whitenoise/ui/contact_list/widgets/share_invite_button.dart'; | ||
| import 'package:whitenoise/ui/contact_list/widgets/share_invite_callout.dart'; | ||
| import 'package:whitenoise/ui/contact_list/widgets/user_profile.dart'; | ||
|
|
@@ -84,15 +86,20 @@ class _StartChatBottomSheetState extends ConsumerState<StartChatBottomSheet> { | |
| nip65Relays: activeAccountData.nip65Relays, | ||
| ); | ||
| final keyPackage = await keyPackageService.fetchWithRetry(); | ||
|
|
||
| if (mounted) { | ||
| setState(() { | ||
| _isLoadingKeyPackage = false; | ||
| _needsInvite = keyPackage == null; | ||
| }); | ||
| } | ||
| } catch (e) { | ||
| _logger.warning('Failed to fetch key package: $e'); | ||
| String error; | ||
| if (e is WhitenoiseError) { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think this is key package service responsibility... it already has a
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Didn't know this exists, I'll check it out. Thanks.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Did it make sense to move it there?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not really. I refactored it and tried to mock the error to make sure I didn't break anything but it's quite tricky. I think I'll just remove it for now and do the refactor if the issue shows up later. It's kinda out of scope for this PR (it's just a side fix) and I don't want to have it delaying this fix. @josefinalliende
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ok, so this if checking the type of error is going to be removed in this PR because it can’t be actually reproduced? |
||
| error = await whitenoiseErrorToString(error: e); | ||
| } else { | ||
| error = e.toString(); | ||
| } | ||
| _logger.warning('Failed to fetch key package: $error'); | ||
| if (mounted) { | ||
| setState(() { | ||
| _isLoadingKeyPackage = false; | ||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||||||
|---|---|---|---|---|---|---|---|---|---|---|
| @@ -1,6 +1,8 @@ | ||||||||||
| import 'package:flutter/material.dart'; | ||||||||||
| import 'package:flutter/services.dart'; | ||||||||||
| import 'package:flutter_screenutil/flutter_screenutil.dart'; | ||||||||||
| import 'package:flutter_svg/svg.dart'; | ||||||||||
| import 'package:whitenoise/ui/core/themes/assets.dart'; | ||||||||||
|
Comment on lines
+4
to
+5
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Use the canonical flutter_svg import to avoid build errors This file uses Apply this diff: -import 'package:flutter_svg/svg.dart';
+import 'package:flutter_svg/flutter_svg.dart';📝 Committable suggestion
Suggested change
🤖 Prompt for AI Agents |
||||||||||
| import 'package:whitenoise/ui/core/themes/src/extensions.dart'; | ||||||||||
| import 'package:whitenoise/ui/core/ui/wn_validation_notification.dart'; | ||||||||||
|
|
||||||||||
|
|
@@ -9,6 +11,11 @@ enum FieldType { | |||||||||
| password, | ||||||||||
| } | ||||||||||
|
|
||||||||||
| enum FieldSize { | ||||||||||
| regular, // 56.h | ||||||||||
| small, // 44.h | ||||||||||
| } | ||||||||||
|
|
||||||||||
| class WnTextFormField extends StatefulWidget { | ||||||||||
| const WnTextFormField({ | ||||||||||
| super.key, | ||||||||||
|
|
@@ -42,6 +49,7 @@ class WnTextFormField extends StatefulWidget { | |||||||||
| this.textCapitalization = TextCapitalization.none, | ||||||||||
| this.autovalidateMode = AutovalidateMode.onUserInteraction, | ||||||||||
| this.inputFormatters, | ||||||||||
| this.size = FieldSize.regular, | ||||||||||
| }); | ||||||||||
|
|
||||||||||
| final Key? formKey; | ||||||||||
|
|
@@ -75,6 +83,7 @@ class WnTextFormField extends StatefulWidget { | |||||||||
| final TextEditingController? controller; | ||||||||||
| final FormFieldValidator<String?>? validator; | ||||||||||
| final InputDecoration? decoration; | ||||||||||
| final FieldSize size; | ||||||||||
|
|
||||||||||
| @override | ||||||||||
| State<WnTextFormField> createState() => _WnTextFormFieldState(); | ||||||||||
|
|
@@ -100,21 +109,14 @@ class _WnTextFormFieldState extends State<WnTextFormField> { | |||||||||
| }); | ||||||||||
| } | ||||||||||
|
|
||||||||||
| Widget? get suffixIcon { | ||||||||||
| final resolvedIcon = ValueListenableBuilder( | ||||||||||
| valueListenable: hasError, | ||||||||||
| builder: (context, hasError, _) { | ||||||||||
| final errorIcon = !hasError ? const SizedBox() : const Icon(Icons.error); | ||||||||||
|
|
||||||||||
| return switch (widget.type) { | ||||||||||
| FieldType.password => errorIcon, | ||||||||||
| FieldType.standard || null => errorIcon, | ||||||||||
| }; | ||||||||||
| }, | ||||||||||
| ); | ||||||||||
|
|
||||||||||
| return widget.decoration?.suffixIcon ?? resolvedIcon; | ||||||||||
| } | ||||||||||
| Widget get suffixIcon => ValueListenableBuilder<bool>( | ||||||||||
| valueListenable: hasError, | ||||||||||
| builder: | ||||||||||
| (_, hasError, _) => | ||||||||||
| hasError | ||||||||||
| ? SvgPicture.asset(AssetsPaths.icErrorFilled) | ||||||||||
| : (widget.decoration?.suffixIcon ?? const SizedBox.shrink()), | ||||||||||
| ); | ||||||||||
|
|
||||||||||
| String? validator(dynamic value) { | ||||||||||
| final result = widget.validator?.call(value); | ||||||||||
|
|
@@ -135,7 +137,13 @@ class _WnTextFormFieldState extends State<WnTextFormField> { | |||||||||
| fontWeight: FontWeight.w600, | ||||||||||
| ); | ||||||||||
|
|
||||||||||
| final isSmall = widget.size == FieldSize.small; | ||||||||||
| final targetHeight = isSmall ? 44.h : 56.h; | ||||||||||
| final bool isMultiline = | ||||||||||
| ((widget.maxLines ?? 1) > 1) || ((widget.minLines ?? 1) > 1) || widget.expands; | ||||||||||
|
|
||||||||||
| final decoration = (widget.decoration ?? const InputDecoration()).copyWith( | ||||||||||
| constraints: isMultiline ? null : BoxConstraints.tightFor(height: targetHeight), | ||||||||||
| suffixIcon: suffixIcon, | ||||||||||
| labelText: widget.labelText, | ||||||||||
| hintText: widget.hintText, | ||||||||||
|
|
@@ -146,10 +154,9 @@ class _WnTextFormFieldState extends State<WnTextFormField> { | |||||||||
| ), | ||||||||||
| suffixIconColor: context.colors.primary, | ||||||||||
| fillColor: context.colors.avatarSurface, | ||||||||||
| contentPadding: EdgeInsets.symmetric( | ||||||||||
| horizontal: 12.w, | ||||||||||
| vertical: 16.h, | ||||||||||
| ), | ||||||||||
| contentPadding: EdgeInsets.symmetric(horizontal: 12.w, vertical: isSmall ? 13.5.h : 19.5.h), | ||||||||||
| prefixIconConstraints: BoxConstraints.tightFor(height: targetHeight), | ||||||||||
| suffixIconConstraints: BoxConstraints.tightFor(height: targetHeight), | ||||||||||
| border: OutlineInputBorder( | ||||||||||
| borderRadius: BorderRadius.zero, | ||||||||||
| borderSide: BorderSide( | ||||||||||
|
|
@@ -165,7 +172,7 @@ class _WnTextFormFieldState extends State<WnTextFormField> { | |||||||||
| filled: true, | ||||||||||
| ); | ||||||||||
|
|
||||||||||
| return TextFormField( | ||||||||||
| final field = TextFormField( | ||||||||||
| key: widget.formKey, | ||||||||||
| validator: validator, | ||||||||||
| enabled: widget.enabled, | ||||||||||
|
|
@@ -195,5 +202,19 @@ class _WnTextFormFieldState extends State<WnTextFormField> { | |||||||||
| keyboardType: widget.keyboardType, | ||||||||||
| inputFormatters: widget.inputFormatters, | ||||||||||
| ); | ||||||||||
|
|
||||||||||
| // If maxLines or minLines is specified, return the field as is | ||||||||||
| // Without using ConstainedBox to enforce the target height. | ||||||||||
| // Same rule applied in InputDecoration above. | ||||||||||
| if (isMultiline) { | ||||||||||
| return field; | ||||||||||
| } | ||||||||||
| // Also enforce the target height at the parent layout level so surrounding | ||||||||||
| // widgets measure consistently. The decoration.constraints above ensures | ||||||||||
| // the border matches this height (no extra whitespace around it). | ||||||||||
| return ConstrainedBox( | ||||||||||
| constraints: BoxConstraints.tightFor(height: targetHeight), | ||||||||||
| child: field, | ||||||||||
| ); | ||||||||||
| } | ||||||||||
| } | ||||||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So, I have a question here, cause my eyes are not detecting the difference between master en this branch locally.... 🙈 where is this border difference? I got some screenshots form different cases, in ios emulator.
Replies cases
masterbranch"Normal" messages cases (without replies)
masterbranchThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Message input focused with content, that's where the issue is. It's pretty tiny, like a pixel or two but it's pretty noticeable too.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But there the fix is the distance between the input and button right? What it has to do with the border changes? Or there are no border changes really?