zulip / zulip-flutter

Future Zulip client using Flutter
Apache License 2.0
159 stars 134 forks source link

login: Link to doc to help users understand what a "server URL" is and how to find theirs #109

Open chrisbobbe opened 1 year ago

chrisbobbe commented 1 year ago

Like we did in zulip/zulip-mobile@52288facc.

VatsalBhesaniya commented 6 months ago

Hi @chrisbobbe, I'm interested in working on this issue.

I have reviewed how this is implemented in zulip/zulip-mobile@52288facc.

I noticed that in Flutter onTap event of the InputDecoration label does not work due to the onClick behavior focusing the text field.

To address this, I propose two possible solutions: Solution 1(Separate Text Widget) We can add a separate Text widget positioned above the TextField, allowing for a tappable link, and do not use the default label from InputDecoration.

https://github.com/zulip/zulip-flutter/assets/38466275/f8e1b616-9afc-4241-808c-899a56acdabe

Solution 2(TextField Suffix) Alternatively, we could leverage a TextField suffix widget containing a link. However, this approach has the limitation of the link being visible only when the field is focused. Alternatively, we can add a suffix widget in TextField containing a link. However, this approach has the limitation of the link being visible only when the TextField is focused.

https://github.com/zulip/zulip-flutter/assets/38466275/82a78d2b-74b5-4a0b-975b-6a516b78b944

I'm open to any improvements to these options and exploring any other suggestions you might have.

chrisbobbe commented 6 months ago

Interesting; thanks for your thoughts on this!

I wonder about using InputDecoration.helperText. The doc says:

Text that provides context about the InputDecorator.child's value, such as how the value will be used.

which seems like a pretty good match for what we want here.

…Hmm, but it looks like that asks for a string, and there isn't a similar parameter to specify a Widget. So it looks like InputDecoration doesn't currently support putting a clickable link in that position. It would be convenient if it had a param with a name like helper, on the pattern of:

If it did, then I think that would be exactly where we'd want to put a "What's this?" link. That would avoid the problem of not being tappable, as in your Solution 1, and also the problem of only being visible when the input was focused, as in your Solution 2.

Would you like to make a PR to the Flutter framework to add that param? I'm hoping that would be straightforward, especially with the commits that added error and label that you could use for reference.

Otherwise, I wonder if it's possible to accomplish something similar without using the InputDecoration API.

VatsalBhesaniya commented 6 months ago

Thanks for your response! Being new to open source, I want to but I might not be ready to contribute to the Flutter framework just yet, still, I will try my best to make a PR to the Flutter framework.

I think we should move forward with Solution 1 for now, as it avoids using the InputDecoration API.

...Wait, I found another solution using suffixIcon. A couple of things to consider with this approach:

  1. We'd need to set floatingLabelBehavior to FloatingLabelBehavior.always to ensure the label remains visible.
  2. The "What's this?" link would be positioned on the right side of the TextField. This approach seems promising!

With FloatingLabelBehavior.always both label text and suffix widget would be perfectly aligned. Screenshot_1710233242

Without FloatingLabelBehavior.always both label text and suffix widget would not be perfectly aligned when TextField is not in focus. Screenshot_1710233410 Screenshot_1710233242

gnprice commented 6 months ago

I agree that adding an InputDecoration.helper field upstream would be the right solution here.

The documented meaning of the existing helperText is exactly the meaning we want here, except only that we need to supply a widget rather than a plain string. And with the existing error, label, prefix, and suffix, there's an established pattern that such a helper field would fit into — so I think people will very easily be convinced that adding such a field is appropriate, and all that's needed is for someone to write up a high-quality PR to do so.

Using suffixIcon for this feels like a hack, because it's far removed from what suffixIcon is intended for. That's going to make it difficult to get the layout and behavior to feel quite right. So I wouldn't want to go down that path.

@VatsalBhesaniya I'd encourage you to go ahead and work on a PR for this for the Flutter framework upstream. The Flutter project does have high standards before merging a PR, but so do we :smile:. Take a look at Flutter's contributor guide: https://github.com/flutter/flutter/blob/master/CONTRIBUTING.md#developing-for-flutter You already actually have a Flutter development tree on your machine, as that's part of installing Flutter. And then see those two previous commits that Chris linked to above (and the PRs that those commits came from — there's links on the commit pages) for examples to work from for what this PR should look like.

If you have questions, please ask in #mobile-team on chat.zulip.org. Both Chris and I have gotten a number of PRs merged into Flutter upstream, so we can give advice. Then when you send the PR, definitely mention it in #mobile-team with a link, and we can take a look and give it its first review.

(And since you'll be writing the PR for Zulip's needs, and we'll be reviewing it, we'll certainly think of it as a contribution to Zulip much the same as we would for a PR in this repo.)

@chrisbobbe would you be up to filing a brief issue for InputDecoration.helper in the upstream tracker? Then @VatsalBhesaniya can link to that when the PR is ready to submit.

VatsalBhesaniya commented 6 months ago

Sure, I agree suffixIcon seems like a workaround. Adding an InputDecoration.helper field upstream sounds like the best approach.

Thank you for your support! I'll try working on a PR for Flutter. I'll keep you informed on the #mobile-team channel.