Add helper text for "server URL" with link to documentation

[VatsalBhesaniya]

Add helper text below the "server URL" field that, when tapped, opens relevant Zulip documentation explaining what a server URL is and how to find theirs. It will help users understand the "server URL" field during login.

Fixes: #109

https://github.com/zulip/zulip-flutter/assets/38466275/2258cbfa-43d6-4339-b50c-c7967ff1472e

https://github.com/zulip/zulip-flutter/assets/38466275/bb9f5144-7b5c-4ea0-bbb1-6a8f59da1cfc

[abelaba]

Hey, so one thing I would say is that your commit message should have a description. As stated here https://zulip.readthedocs.io/en/latest/contributing/commit-discipline.html#commit-description

[abelaba]

You must also add new tests for your code so that maintainers will review your code in detail. https://github.com/zulip/zulip-flutter?tab=readme-ov-file#writing-tests

[VatsalBhesaniya]

Hey, so one thing I would say is that your commit message should have a description. As stated here https://zulip.readthedocs.io/en/latest/contributing/commit-discipline.html#commit-description

@abelaba Thank you for your feedback. 🙂

[VatsalBhesaniya]

Thanks @VatsalBhesaniya, I'm looking forward to this! This will help people get what they need to start using the app.

Small comments below. Also, this will need tests. Here are the things I would test:

• Basic happy case: When the text is tapped, the URL is opened. For this, see our uses of testBinding.takeLaunchUrlCalls().
• Error case: When the URL fails to be opened, we show an error dialog. For this, see our uses of testBinding.launchUrlResult and checkErrorDialog.

Also, please mark the commit with a Fixes line. I see you've already done this in the GitHub PR description. 🙂

Thanks for the positive feedback! 🙂 I appreciate your suggestions on tests. I've added tests covering the success and the error cases.

I've also updated the commit message to include the Fixes line.

Please let me know if you have any further feedback or require any additional changes to the tests.

[chrisbobbe]

Hi! One small comment I meant to leave last time, and I think Greg intends to do the next full review. 🙂

There's still a TODO(#109) in the code that should be removed in the commit that's marked as fixing #109. I've proposed an automated way to catch things like this:

• #599

[VatsalBhesaniya]

Thanks @VatsalBhesaniya for the revision!

In this version, the first commit introduces a duplicate _launchUrl function which is missing the mode logic, much like in the previous revision as discussed at #595 (comment) . Then the second commit deduplicates the two functions into one combined place.

As part of writing clear and coherent commits, we want to avoid that intermediate step where we're adding a _launchUrl function that has the wrong behavior. So instead a good commit sequence would be:

• Move the existing _launchUrl to the new file, renaming it, perhaps splitting into pieces, but maintaining the exact same behavior. This will be an NFC commit.
• Add i18n for that function.
• Add the UI that resolves login: Link to doc to help users understand what a "server URL" is and how to find theirs #109, calling that function.

After that's done it'll again be possible to do a fine-grained review. In the meantime, here's a couple of comments from a high-level look.

Thank you for your detailed feedback and guidance!

I have followed your suggested approach and updated the commits accordingly. Let me know if you have any further suggestions.

[chrisbobbe]

Hi @VatsalBhesaniya, it looks like you've updated the PR branch. Is it ready for another review? When you push updates and it's ready for another review, it's helpful if you post a comment saying so, for explicitness; that way reviewers don't have to guess. 🙂

[VatsalBhesaniya]

Hi @VatsalBhesaniya, it looks like you've updated the PR branch. Is it ready for another review? When you push updates and it's ready for another review, it's helpful if you post a comment saying so, for explicitness; that way reviewers don't have to guess. 🙂

Hi @chrisbobbe, Yes, the PR branch is ready for another review! I apologize for the confusion. I made updates locally, but for some reason, they weren't reflecting on GitHub 🤔. This hasn't happened to me before, so I hesitated to request another review until I was certain everything was pushed correctly. Now I can see all of my updates. 🙂

[chrisbobbe]

Hi @VatsalBhesaniya, are you planning to return to this?

[VatsalBhesaniya]

Hi @VatsalBhesaniya, are you planning to return to this?

Sorry for the delay. I have made all the changes requested.

[chrisbobbe]

Great, thank you! This revision LGTM; I'll mark this for Greg's review now.

[alya]

I don't understand the "Unable to open link" screenshots in the PR description. Is that something users will see?

[PIG208]

Hi @VatsalBhesaniya! Thanks for the previous revisions. Any plan to continue working on this?

[gnprice]

Closing this PR to get it out of our queue. Thanks @VatsalBhesaniya for your work on it so far.

The existing PR branch will remain available in Git, for the next person to refer to when picking up this issue #109, and it has the "completion candidate" label which may make easier to find for that purpose. I'll also make a note on the issue.