search

zulip / zulip

Zulip server and web application. Open-source team chat that helps teams stay productive and focused.
https://zulip.com
Apache License 2.0
21.18k stars 7.66k forks source link

Extract logic for creating links to messages. #21412

Open Fingel opened 2 years ago

Fingel commented 2 years ago

Currently the Zulip mobile app is lacking the ability to copy a link to a specific message. #3865 has been tracking that issue. Ideally, we would be able to share the logic in Zulip web for constructing these links with Zulip mobile, to avoid duplication and improve consistency.

Recently, there has been some work done to share code between the Zulip web and mobile, specifically code related to generating links in both platforms.

https://github.com/zulip/zulip/pull/21272 is a pull request that moved several functions into a new module called internal_url. The mobile app was then able to use this code to implement "Copy link to topic" and "Copy link to stream" actions. See https://github.com/zulip/zulip-mobile/pull/5182 and https://github.com/zulip/zulip-mobile/pull/5279 for the implementation of those.

We would like to take the same approach to implementing the copy link to message action. If we take a look at the implementation on the web, the main entrypoint for creating a link a message narrow is here:

https://github.com/zulip/zulip/blob/560cf06b7280d420c120e34e621fd27ac8650dbb/static/js/hash_util.js#L133

There are a few refactors that will need to occur to make this code shareable. One of the first and most obvious is the use of window.location, which doesn't make sense in a mobile context.

There is a call into the people module here which would also need to be refactored.

The code we want to refactor most likely references web specific data structures. The way we dealt with that in sharing previous methods was to use callbacks which both the mobile app and web app can pass in to, for example, get the name of a stream by it's ID.

One caveat of this approach is that it may lead to the proliferation of additional method arguments all over the code. To lessen the need for this, it is possible to leave wrapper functions in the original location, so that none of the platform specific calls need to change.

Once the code is refactored and moved it would be beneficial to make sure the tests and flow definitions are up to date to ensure a smooth incorporation in to the mobile app.

zulipbot commented 2 years ago

Hello @zulip/server-refactoring members, this issue was labeled with the "area: refactoring" label, so you may want to check it out!

rskbansal commented 2 years ago

@zulipbot claim

zulipbot commented 2 years ago

Welcome to Zulip, @rskbansal! We just sent you an invite to collaborate on this repository at https://github.com/zulip/zulip/invitations. Please accept this invite in order to claim this issue and begin a fun, rewarding experience contributing to Zulip!

Here's some tips to get you off to a good start:

As you work on this issue, you'll also want to refer to the Zulip code contribution guide, as well as the rest of the developer documentation on that site.

See you on the other side (that is, the pull request side)!

zulipbot commented 2 years ago

Hello @rskbansal, you have been unassigned from this issue because you have not updated this issue or any referenced pull requests for over 14 days.

You can reclaim this issue or claim any other issue by commenting @zulipbot claim on that issue.

Thanks for your contributions, and hope to see you again soon!

itsmedeepak commented 2 years ago

@zulipbot claim

zulipbot commented 2 years ago

Hello @itsmedeepak, it looks like someone has already claimed this issue! Since we believe multiple assignments to the same issue may cause some confusion, we encourage you to search for other unclaimed issues to work on. However, you can always reclaim this issue if no one is working on it.

We look forward to your valuable contributions!

mschuler10 commented 1 year ago

@zulipbot claim

zulipbot commented 1 year ago

Welcome to Zulip, @mschuler10! We just sent you an invite to collaborate on this repository at https://github.com/zulip/zulip/invitations. Please accept this invite in order to claim this issue and begin a fun, rewarding experience contributing to Zulip!

Here's some tips to get you off to a good start:

As you work on this issue, you'll also want to refer to the Zulip code contribution guide, as well as the rest of the developer documentation on that site.

See you on the other side (that is, the pull request side)!

zulipbot commented 1 year ago

@mschuler10 You have been unassigned from this issue because you have not made any updates for over 14 days. Please feel free to reclaim the issue if you decide to pick up again. Thanks!

mschuler10 commented 1 year ago

@zulipbot claim

zulipbot commented 1 year ago

Hello @mschuler10, it looks like you've currently claimed 1 issue in this repository. We encourage new contributors to focus their efforts on at most 1 issue at a time, so please complete your work on your other claimed issues before trying to claim this issue again.

We look forward to your valuable contributions!

zulipbot commented 1 year ago

Hello @mschuler10, it looks like you've currently claimed 1 issue in this repository. We encourage new contributors to focus their efforts on at most 1 issue at a time, so please complete your work on your other claimed issues before trying to claim this issue again.

We look forward to your valuable contributions!

zulipbot commented 1 year ago

Hello @mschuler10, it looks like you've currently claimed 1 issue in this repository. We encourage new contributors to focus their efforts on at most 1 issue at a time, so please complete your work on your other claimed issues before trying to claim this issue again.

We look forward to your valuable contributions!

zulipbot commented 1 year ago

ERROR: You have not claimed this issue to work on yet.

zulipbot commented 1 year ago

Hello @mschuler10, it looks like you've currently claimed 1 issue in this repository. We encourage new contributors to focus their efforts on at most 1 issue at a time, so please complete your work on your other claimed issues before trying to claim this issue again.

We look forward to your valuable contributions!

schulerMartins commented 1 year ago

@zulipbot claim

zulipbot commented 1 year ago

Welcome to Zulip, @schulerMartins! We just sent you an invite to collaborate on this repository at https://github.com/zulip/zulip/invitations. Please accept this invite in order to claim this issue and begin a fun, rewarding experience contributing to Zulip!

Here's some tips to get you off to a good start:

As you work on this issue, you'll also want to refer to the Zulip code contribution guide, as well as the rest of the developer documentation on that site.

See you on the other side (that is, the pull request side)!

schulerMartins commented 1 year ago

@zulipbot abandon

mschuler10 commented 1 year ago

@zulipbot claim

zulipbot commented 1 year ago

@mschuler10 You have been unassigned from this issue because you have not made any updates for over 14 days. Please feel free to reclaim the issue if you decide to pick up again. Thanks!

mschuler10 commented 1 year ago

@mschuler10 We noticed that you have not made any updates to this issue or linked PRs for 10 days. Please comment here if you are still actively working on it. Otherwise, we'd appreciate a quick @zulipbot abandon comment so that someone else can claim this issue and continue from where you left off.

If we don't hear back, you will be automatically unassigned in 4 days. Thanks!

Hi, I have submitted a PR for this issue and waiting for a review!

mschuler10 commented 1 year ago

@zulipbot claim

zulipbot commented 1 year ago

@mschuler10 We noticed that you have not made any updates to this issue or linked PRs for 10 days. Please comment here if you are still actively working on it. Otherwise, we'd appreciate a quick @zulipbot abandon comment so that someone else can claim this issue and continue from where you left off.

If we don't hear back, you will be automatically unassigned in 4 days. Thanks!

gnprice commented 1 year ago

Marked this as no longer "help wanted" because we're building a new Zulip mobile client in Flutter to replace the React Native app, as a result of which the shared package isn't going to be used by mobile in the future and so moving functionality into it isn't needed.

(It looks like this was the only "help wanted" issue in the tracker for doing this kind of move.)

mschuler10 commented 1 year ago

@zulipbot abandon