Skip to content
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

Add follow/unfollow options to topic action sheet #5794

Merged
merged 14 commits into from
Nov 22, 2023

Conversation

gnprice
Copy link
Member

@gnprice gnprice commented Nov 17, 2023

Stacked atop #5793.

Fixes: #5771

Is it possible that this test file has never actually been
getting run by our test suite?  Yikes.  Now it does.
This patch is a backport of commit facebook/react-native@cf631ad59
from upstream.  I've also pushed it to a branch in our fork:
  zulip/react-native@v0.68.7...0.68.7-zulip

In particular this avoids a symptom which I otherwise see
on Node v18.16.1:

```
 FAIL  src/webview/__tests__/generateInboundEventEditSequence-test.js
  ● Test suite failed to run

    TypeError: Cannot redefine property: performance

      at Object.<anonymous> (node_modules/react-native/jest/setup.js:20:20)
```

The symptom affects three of our test files:

    src/__tests__/test-test.js
    src/webview/__tests__/generateInboundEventEditSequence-test.js
    src/webview/js/__tests__/rewriteHtml-test.js

because those are the three that have `@jest-environment` annotations
calling for "jsdom" or related environments.
Done by rerunning tools/build-icon-font .

Ideally this should have happened at the same time as upgrading
`@zulip/shared`, so in commit 9716c9e and/or previous upgrades.
This SVG file is copied verbatim from the web app.  Ideally we'd
do that by upgrading `@zulip/shared`... but now that this codebase
is legacy and we're spending less time maintaining it, we've
accumulated a backlog of changes there that are incompatible and
require adjustments in our code.  So the expedient thing is to
just take the individual icon we currently need and pull it into
our tree directly.

Moreover, we want this icon inside the webview.  This makes only
the second Zulip icon we've put inside the webview, after the
"play_button" icon we added just the other day; and I wouldn't say
we've developed a good streamlined pattern for it.  But for this
legacy codebase, we'll content ourselves with just following the
pattern set by "play_button".

That also means duplicating the icon's SVG file into two places.
Awkward, but at least it's not any kind of space-consumption issue:
the file is under a kilobyte in size.
This is most visible on the inbox screen, the app's home screen.
It also appears on the screen that lists the topics for a stream.
As discussed back in 4851f34, the values get stringified either
by `lodash.escape` or by the `join`.  Stringifying a boolean works
just as nicely as a number.
Together with the preceding few commits which add this icon in
some other places in the UI, this completes zulip#5770.

Fixes: zulip#5770
These are the current values on chat.zulip.org.
@gnprice gnprice added webapp parity Features that exist in the webapp that we need to port to mobile. We aren't aiming for full parity. server release goal Things we should try to coordinate with a major Zulip Server release. labels Nov 17, 2023
@chrisbobbe chrisbobbe merged commit ab99bfe into zulip:main Nov 22, 2023
1 check passed
@chrisbobbe
Copy link
Contributor

Thanks! Merged.

@gnprice gnprice deleted the pr-follow-topic-option branch November 22, 2023 19:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
server release goal Things we should try to coordinate with a major Zulip Server release. webapp parity Features that exist in the webapp that we need to port to mobile. We aren't aiming for full parity.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Make it possible to follow/unfollow a topic
2 participants