-
Notifications
You must be signed in to change notification settings - Fork 321
Set status page adjustments #1800
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
base: main
Are you sure you want to change the base?
Conversation
a00e41d
to
07bf76c
Compare
Thanks! Could you post before-and-after screenshots? For #1771 I see screenshots of two proposed fixes, but they only show what the button looks like when it's pressed, which is not the common case. For whichever proposal is implemented here, could you please include a screenshot with and without that touch feedback? |
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.
Thanks! And here are some comments on the code.
lib/widgets/set_status.dart
Outdated
top: 8, end: 10, | ||
// In Figma design, this is 4px, be we compensate for that in | ||
// [SingleChildScrollView.padding] below. | ||
bottom: 0), | ||
child: Row( | ||
crossAxisAlignment: CrossAxisAlignment.start, | ||
children: [ | ||
IconButton( | ||
onPressed: chooseStatusEmoji, | ||
style: IconButton.styleFrom( | ||
splashFactory: NoSplash.splashFactory, | ||
foregroundColor: designVariables.icon, | ||
tapTargetSize: MaterialTapTargetSize.shrinkWrap, | ||
padding: EdgeInsets.symmetric( | ||
vertical: 8, | ||
// In Figma design, there is no horizontal padding, but we | ||
// provide it in order to create a proper tap target size. | ||
horizontal: 8)), | ||
icon: Row(spacing: 4, children: [ | ||
ValueListenableBuilder( | ||
valueListenable: statusChange, | ||
builder: (_, change, _) { | ||
final emoji = change.emoji.or(oldStatus.emoji); | ||
return emoji == null | ||
? const Icon(ZulipIcons.smile, size: 24) | ||
: UserStatusEmoji(emoji: emoji, size: 24, neverAnimate: false); | ||
}), | ||
Icon(ZulipIcons.chevron_down, size: 16), | ||
]), | ||
), | ||
Expanded(child: TextField( | ||
controller: statusTextController, | ||
minLines: 1, | ||
maxLines: 2, | ||
// The limit on the size of the status text is 60 characters. | ||
// See: https://zulip.com/api/update-status#parameter-status_text | ||
maxLength: 60, | ||
cursorColor: designVariables.textInput, | ||
textCapitalization: TextCapitalization.sentences, | ||
style: TextStyle(fontSize: 19, height: 24 / 19), | ||
decoration: InputDecoration( | ||
// TODO: display a counter as suggested in CZO discussion: | ||
// https://chat.zulip.org/#narrow/channel/530-mobile-design/topic/Set.20user.20status/near/2224549 | ||
counterText: '', | ||
hintText: localizations.statusTextHint, | ||
hintStyle: TextStyle(color: designVariables.labelSearchPrompt), | ||
isDense: true, | ||
contentPadding: EdgeInsets.symmetric( | ||
vertical: 8, | ||
// Subtracting 4 pixels to account for the internal | ||
// 4-pixel horizontal padding. | ||
horizontal: 10 - 4, | ||
), | ||
filled: true, | ||
fillColor: designVariables.bgSearchInput, | ||
border: OutlineInputBorder( | ||
borderRadius: BorderRadius.circular(10), | ||
borderSide: BorderSide.none, | ||
)))), | ||
]), | ||
), | ||
body: SafeArea( | ||
bottom: false, | ||
minimum: EdgeInsets.symmetric(horizontal: 8), | ||
child: Column(children: [ | ||
Padding( | ||
padding: const EdgeInsetsDirectional.only( | ||
// In Figma design, this is 16px, but we compensate for that in | ||
// the icon button below. | ||
start: 8, | ||
top: 8, end: 8, |
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.
This is a good change (10px to 8px, as I suggested in #1769), but not related to rendering in landscape mode, which is what the commit says it's about.
lib/widgets/set_status.dart
Outdated
return InkWell( | ||
onTap: onTap, | ||
splashFactory: NoSplash.splashFactory, | ||
overlayColor: WidgetStateColor.resolveWith( | ||
(states) => states.any((e) => e == WidgetState.pressed) | ||
? designVariables.contextMenuItemBg.withFadedAlpha(0.20) | ||
: Colors.transparent, | ||
), | ||
overlayColor: WidgetStatePropertyAll(Colors.transparent), |
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.
Is the InkWell
doing anything, now, that a plain GestureDetector
wouldn't?
lib/widgets/set_status.dart
Outdated
? designVariables.contextMenuItemBg.withFadedAlpha(0.20) | ||
: Colors.transparent, | ||
), | ||
overlayColor: WidgetStatePropertyAll(Colors.transparent), | ||
child: Padding( | ||
padding: EdgeInsets.symmetric(vertical: 7, horizontal: 16), |
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.
From the issue:
- For all page content, take 8px of horizontal external whitespace and move it to the outer
SafeArea.minimum
But it looks like this commit just adds 8px of horizontal whitespace, without subtracting it from anywhere.
3c6a36c
to
b01c643
Compare
Thanks @chrisbobbe for the review. New revision pushed. Screenshots are also included in the PR description. |
Thanks! I'd like the first three commits to be more clear and coherent: 856df7d set-status: Fix content rendering in landscape mode The first says it fixes something, but it also creates a problem (or at least unintended behavior): in iPhone landscape mode there's more horizontal padding than we want: ![]() That's because this part of the issue hasn't been done yet (at that commit):
so that 8px is added unconditionally, even though the horizontal insets are greater than 8px. That problem is fixed in the second commit— ![]() —but that fix isn't described in the commit message, and in any case it's neater to not introduce the problem in the first place. 🙂 The third commit could helpfully say why we're removing the touch feedback. Could just quote my reasoning in #1769 🙂. Also a nit: best to remove the touch feedback either before or in the commit that causes it to not extend to the screen edges. Design-wise, I think it looks weird for a touchable row to have non-rounded rectangular touch feedback that doesn't align with some persistently visible edge, like the edge of a row's distinctly colored surface or the edge of the screen. This is actually visible in your "Before" screenshot of the touch feedback, but not in Anyway, how's this for a commit sequence:
The other two commits LGTM: 52d4f16 set-status: Remove shadow effect at the bottom of the page |
b01c643
to
93aee77
Compare
As Chris mentioned in zulip#1769 description: "Change the text-input row's end margin from 10px to 8px, to match the 8px end margin on the status suggestions (I know this goes against the Figma but I'd prefer consistency 🙂)"
As Chris mentioned in zulip#1769 description: "Remove the InkWell touch feedback on status-suggestion items. I usually like touch feedback, but the Figma doesn't specify it here, and it's not clear if we'd want it to extend through the device insets or not. Thankfully there's no network request so it should always be immediately clear that a tap was detected (the inputs will change)."
93aee77
to
069e10d
Compare
Thanks for the review. Pushed new changes. |
Before this in landscape mode, the content would extend and partially hide beneath the top notch. Now the content is displayed correctly and starts just from below the notch. Fixes: zulip#1769
This will separate the emoji button from text input field when emoji button is pressed (with touch feedback shown). Fixes: zulip#1771
069e10d
to
a1b344c
Compare
Fixes: #1769
Fixes: #1770
Fixes: #1771
Screenshots
Landscape mode
Consistent horizontal padding
Status-suggestion touch feedback
Bottom shadow
Space between emoji button and text input
Emoji button pressed
Emoji button not pressed