Show "followed topic" icon in several places #5793
Conversation
gnprice
added
webapp parity
|
Hmm, looks like some CI failures. (Commenting because I'm not sure if you get a notification when CI fails.) |
|
Indeed, thanks — I don't. Will fix. |
gnprice
force-pushed
the
pr-follow-topic
branch
from
6ae2038 to
e669038
Compare
|
Pushed a change which should fix that. |
|
@alya @terpimost for feedback on the UI; see screenshots above. Anything complex, let's save for the Flutter app where we'll be much more able to control things to be just how we want them. But size, padding, opacity, color are all just numbers and are easy to adjust from this draft's values to any other values. |
|
I would maybe reduce opacity in the app bar, but very happy to defer to @terpimost on any tweaks here. |
|
Looks like still some CI failures, in message-list snapshot tests. |
Hmmm indeed. Now why wasn't that failing for me locally… … aha, it's because it's in the same test file where I have unrelated breakage: (I think it may have been induced by a Node upgrade?) and I've gotten used to ignoring that. A parable, I guess. (That issue only started sometime after we stopped focusing on this codebase, which is why I didn't promptly prioritize clearing it up.) Anyway, will fix. |
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.
gnprice
force-pushed
the
pr-follow-topic
branch
from
e669038 to
bce4032
Compare
|
OK, revision pushed. This includes a couple of new commits at the start to fix that other issue I was seeing locally. |
gnprice
force-pushed
the
pr-follow-topic
branch
from
bce4032 to
84ee5c5
Compare
|
Just pushed another revision that fixes the last commit (the one about recipient headers) so that it properly updates if you follow or unfollow a topic. (The lack of such an update is a glitch one's unlikely to notice as a user as of the tip of this PR, but starts becoming noticeable with #5771, if you try following/unfollowing a topic from the long-press menu on a recipient header itself.) |
|
Thanks! LGTM; looks like it's just pending a decision on some opacity: #5793 (comment) |
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
gnprice
force-pushed
the
pr-follow-topic
branch
from
84ee5c5 to
4ef0a0a
Compare
|
OK, I've cranked down that opacity to 0.4. Here's an update of the screenshot: Happy to tweak further, either now or after merging. |
|
Looks fine to me! It's probably reasonable to merge, and make any tweaks as a follow-up when @terpimost has time to provide feedback. |
Fixes #5770.
For the design in the inbox screen and the recipient headers, we use the current web design as a reference. For the app bar it's a bit more arbitrary.
Screenshots