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.

Sign up for GitHub

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

Jump to bottom

Use "list" icon for all-messages, matching web's new icon there #5303

Open
gnprice opened this issue Mar 17, 2022 · 16 comments · Fixed by #5458 · May be fixed by #5532
Open

Use "list" icon for all-messages, matching web's new icon there #5303

gnprice opened this issue Mar 17, 2022 · 16 comments · Fixed by #5458 · May be fixed by #5532
Assignees
@aritroCoder
Labels
good first issue help wanted

Comments

@gnprice
Copy link
Member

gnprice commented Mar 17, 2022 •
edited
Loading

We currently use a "globe" icon for the button on the main screen that takes you to the all-messages view. This has never been a great icon to use there, but we didn't feel we had a better one -- the web app used a "home" icon, and that seemed like it would put too much emphasis on that view. (Unlike on web, the all-messages view has never been the main view that we present.)

But now web uses a "list" icon:
image

That seems perfectly reasonable, and it's helpful to have the same icon on web as on mobile. So let's switch to that one.

(From chat thread here.)
@Vatsalsoni13
Copy link

Can I work on this?

@wilsonfurtado2000 wilsonfurtado2000 mentioned this issue Mar 19, 2022
Closed
@wilsonfurtado2000
Copy link

I have created a PR please look into it

@avdhendra
Copy link

i can change that icon can you assign me on this

@dootMaster
Copy link

Hi, I'd like to help out on this issue.

@alya
Copy link
Collaborator

alya commented Jul 26, 2022

@dootMaster great! Please post a comment describing your proposed approach when you're ready. We can assign the issue to you once you have a rough plan. Thanks!

@dootMaster
Copy link

dootMaster commented Jul 30, 2022 •
edited
Loading

@alya

The goal is to change the icon of mobile Zulip's All Messages tab to be congruent with the icon that currently exists in the browser Zulip app.

  • My investigation into the codebase shows me that HomeScreen.js contains a button component titled TopTabButton.
  • TopTabButton accepts a string property name that is passed into another component titled Icon.
  • The Icon component is sourced from Icons.js, a script that contains numerous functions that help build icons from the react-native-vector-icons package dependency.

Rough plan:

First, I will read the docs: Contributing, and Style Guide.

  1. Change the string property name passed in the TopTabButton component such that when it is consumed by the Icon function in Icons.js, an icon that is congruent to the "list" icon that currently exists on the browser Zulip app is returned.
  2. Run unit tests.
  3. Submit a PR and submit to code review stream on chat.zulip.org.

@chrisbobbe
Copy link
Contributor

Sounds good, thanks, @dootMaster! I've just assigned this issue to you.

@dootMaster dootMaster mentioned this issue Aug 3, 2022
Closed
@dootMaster
Copy link

dootMaster commented Aug 3, 2022 •
edited
Loading

I've created a draft pull request. I'll also post this in the code review stream on the chat.

edit: tag for visibility @chrisbobbe

This was referenced Aug 4, 2022
Closed
Merged
@dootMaster dootMaster mentioned this issue Aug 20, 2022
Closed
dootMaster pushed a commit to dootMaster/zulip-mobile that referenced this issue Aug 20, 2022
homescreen_icon: Swap All-Messages icon from 'globe' to 'list'.
4a659c6 
to match mobile's All Messages icon with the one found in web Zulip.

Fixes: zulip#5303
@dootMaster dootMaster mentioned this issue Aug 20, 2022
Closed
@aritroCoder
Copy link

I want to work on this issue, please assign it to me

@chrisbobbe
Copy link
Contributor

chrisbobbe commented Nov 2, 2022 •
edited
Loading

Sure, I see the current PR #5471 has been awaiting a revision from the author for some time, but it looks like we have a proposed implementation; see #5471 (comment). You can try doing that.

@chrisbobbe chrisbobbe assigned aritroCoder and unassigned dootMaster Nov 2, 2022
dootMaster pushed a commit to dootMaster/zulip-mobile that referenced this issue Nov 3, 2022
homescreen_icon: Swap icon 'globe' with 'list' for web UI parity.
edc9835 
Web Zulip's all-messages icon is an inverted list icon, so we're
swapping mobile's all-messages icon to match.

Fixes: zulip#5303
dootMaster pushed a commit to dootMaster/zulip-mobile that referenced this issue Nov 3, 2022
homescreen_icon: Swap icon 'globe' with 'list' for web UI parity.
0049773 
Web Zulip's all-messages icon is an inverted list icon, so we're
swapping mobile's all-messages icon to match.

Fixes: zulip#5303
@dootMaster
Copy link

Hi, things got a little busy for me. The revision is done; I've created a PR awaiting review.

@alya
Copy link
Collaborator

alya commented Nov 3, 2022

@dootMaster What's the right PR to look at? #5458? (I guess GitHub incorrectly show it as merged?)

@dootMaster
Copy link

@alya #M5532. Yeah, GitHub is incorrectly showing the other as merged.

dootMaster pushed a commit to dootMaster/zulip-mobile that referenced this issue Nov 8, 2022
homescreen_icon: Swap icon 'globe' with 'list' for web UI parity.
441b6d9 
Web Zulip's all-messages icon is an inverted list icon, so we're
swapping mobile's all-messages icon to match.

Fixes: zulip#5303
dootMaster pushed a commit to dootMaster/zulip-mobile that referenced this issue Nov 8, 2022
homescreen_icon: Swap icon 'globe' with 'list' for web UI parity.
f33b6b3 
Web Zulip's all-messages icon is an inverted list icon, so we're
swapping mobile's all-messages icon to match.

Fixes: zulip#5303
dootMaster pushed a commit to dootMaster/zulip-mobile that referenced this issue Nov 8, 2022
homescreen_icon: Swap icon 'globe' with 'list' for web UI parity.
eaf8e09 
Web Zulip's all-messages icon is an inverted list icon, so we're
swapping mobile's all-messages icon to match.

Fixes: zulip#5303
@gnprice gnprice linked a pull request Dec 1, 2022 that will close this issue
Open
@entrymaster
Copy link

I checked the app, we still have this issue. I want to work on this.

@alya
Copy link
Collaborator

alya commented Feb 22, 2023

I think the next step here is for maintainers to re-review #5532.

@ashutosh887
Copy link

@alya
Is it resolved?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@aritroCoder aritroCoder

Labels
good first issue help wanted
Projects
None yet
Milestone
No milestone
10 participants
@gnprice @alya @chrisbobbe @Vatsalsoni13 @entrymaster @dootMaster @avdhendra @ashutosh887 @wilsonfurtado2000 @aritroCoder