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

message list: Show an actual star for starred messages #3593

Open
gnprice opened this issue Aug 22, 2019 · 7 comments
Open

message list: Show an actual star for starred messages #3593

gnprice opened this issue Aug 22, 2019 · 7 comments

Comments

@gnprice
Copy link
Member

gnprice commented Aug 22, 2019

Following up on #2627 :

In the message list, we should display an indication of whether a message has been starred. For consistency and ease of understanding, it should look a lot like what we do in the webapp, which is a "star" icon in the upper-right of the message.

We actually already had code to try to show this, but the mechanics of updating the data to drive that were buggy. That's #2676 and was fixed.

Still, UX-wise the small and faint word "starred" leaves room for improvement. This issue is to replace it with an actual star.

Steps to do that will look like:

  • Consult the webapp to see what HTML and CSS it uses for this case. The source code will likely be all under static/ in zulip/zulip. I'd start by inspecting a starred Zulip message in the Chrome DevTools (or another browser's equivalent) to see the final HTML and the relevant CSS rules.
  • Add needed bits of HTML to messageAsHtml.js, and remove the starred bit from messageTagsAsHtml there.
  • Add needed bits of CSS to css.js, and to cssNight.js if relevant.
  • Use a desktop browser's dev tools to inspect and fiddle with the result. See our doc here: https://github.com/zulip/zulip-mobile/blob/master/docs/howto/debugging.md#webview
@chrisbobbe
Copy link
Contributor

chrisbobbe commented Jan 9, 2020

Would you say the star should be tappable to toggle starred/unstarred, or is that too small of a target for people to interact with? Currently most message actions are done by long-pressing a message, which makes sense, but I think users generally expect a star on a message to be tappable, as in Gmail, for example.

@rk-for-zulip
Copy link
Contributor

My instinct here is that associating an always-visible ☆ or ★ with every message will exacerbate issue #3632, although tapping a star just to remove it probably wouldn't be a bad idea.

Unrelatedly, I wouldn't necessarily suggest trying to use the webapp's star code: it uses FontAwesome, and while we have that in the app already, I recall there being some annoying subtleties involved in making that font accessible to the WebView on iOS.

@chrisbobbe
Copy link
Contributor

chrisbobbe commented Jan 9, 2020

Hmm, are you mostly concerned with adding unused vertical space? An always-visible ☆ or ★ wouldn't necessarily have to add vertical space; it could go on the left of the timestamp, as in the web app (as viewed from either mobile or desktop), right:

image

As a side note for the web app (EDIT: filed as zulip/zulip#13642), it does appear that in the mobile view, those three buttons to the left of the timestamp are invisible until you tap that area for the first time on a message, but when you do tap that invisible area, the buttons respond the same as if they had been visible. For example, I could accidentally tap that empty area, not knowing those buttons were there, and star a message or open a menu by mistake. It's not a huge issue, but it seems slightly incomplete as a way to handle the lack of hover interactions on touch screens. It's also the case that someone using the web app on mobile might never tap that empty area and so not be aware of all the things they can do to a message (edit, quote, star, collapse, etc.).

@rk-for-zulip
Copy link
Contributor

(From offline, for the record:)

Not vertical specifically, but yes – and about stealing existing space from text, which would probably also not be great. Placing it to the left of the timestamp would be fine for leading messages, but immediately-subsequent messages by the same user don't have always-visible timestamps; you'd have to do something different for those.

There are many possibilities, though; I'm sure something workable can be found.

@gnprice
Copy link
Member Author

gnprice commented Jan 10, 2020

Would you say the star should be tappable to toggle starred/unstarred, or is that too small of a target for people to interact with? Currently most message actions are done by long-pressing a message, which makes sense, but I think users generally expect a star on a message to be tappable, as in Gmail, for example.

Yeah, I agree users will generally expect it to be tappable, and it'd be good to make it tappable.

We should make the touch target big enough. A common pattern is to have a touch target that includes some padding around the icon that identifies it. I'd consult the Material Design guidelines for a value for how big the touch target should be.

(Though also, as long as we also leave the long-press approach in place, it's OK if the star's touch target is smaller than spec.)

@chrisbobbe chrisbobbe added the blocked on other work To come back to after another related PR, or some other task. label Jan 23, 2020
@chrisbobbe
Copy link
Contributor

chrisbobbe commented Jan 23, 2020

From our chats in person, we concluded that it would be good to use FontAwesome to be consistent with the web app, but an easy way to do this is blocked by #3548 #3744.

Because it addresses such a similar UI area, I have to say this is also blocked by #3488 (comment) — but that may be an orphaned PR that just needs to be closed, but I'm not sure (see my comment there).

@rk-for-zulip
Copy link
Contributor

Erratum: the actual blocker for the easy way to use FontAwesome is PR #3744, which augments the build-webview script to be (relatively) easily extensible to include new webview asset groups. (In this case, an internal relative symlink to FontAwesome.)

@gnprice gnprice removed the blocked on other work To come back to after another related PR, or some other task. label Jun 8, 2020
@chrisbobbe chrisbobbe removed their assignment Jun 4, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants