homescreen_icon: Change 'globe' to 'list' #5532
Conversation
dootMaster
force-pushed
the
pr-homescreen-icon-change-5303
branch
from
edc9835
to
0049773
Compare
dootMaster
changed the title
dootMaster
changed the title
There was a problem hiding this comment.
Thanks @dootMaster!
This doesn't actually work for me in my testing -- the icon is blank, or missing:
See comments below, including one that I think diagnoses why that's not working.
src/main/HomeScreen.js
Outdated
| <TopTabButton | ||
| <TopTabButtonGeneral | ||
| name="globe" | ||
| onPress={() => { | ||
| dispatch(doNarrow(HOME_NARROW)); | ||
| }} | ||
| /> | ||
| > | ||
| <Icon size={24} style={{ textAlign: 'center ' }} color={BRAND_COLOR} name="globe" /> |
There was a problem hiding this comment.
Good to split this NFC change out as its own commit -- that's a helpful simplification for the main commit.
For that to do its job of making the changes easier to read and understand, though, it's important that each individual commit is correct and coherent:
https://zulip.readthedocs.io/en/latest/contributing/version-control.html
As it stands, the app actually crashes at this commit. If you run Flow while at this commit, it'll point out the cause of that crash, and also another type error.
src/common/Icons.js
Outdated
| @@ -108,3 +108,4 @@ export const IconGroup: SpecificIconType = makeIcon(FontAwesome, 'group'); | |||
| export const IconPlus: SpecificIconType = makeIcon(Feather, 'plus'); | |||
| // eslint-disable-next-line react/function-component-definition | |||
| export const IconWebPublic: SpecificIconType = props => <ZulipIcon name="globe" {...props} />; | |||
| export const IconAllMessages: SpecificIconType = props => makeIcon(FontAwesome, 'align-left'); | |||
There was a problem hiding this comment.
Take a look again at how this line compares to the existing examples in the file. I think this is the reason why this version doesn't work, showing a blank icon.
src/main/HomeScreen.js
Outdated
| <Icon size={24} style={{ textAlign: 'center ' }} color={BRAND_COLOR} name="globe" /> | ||
| <IconAllMessages | ||
| size={24} | ||
| style={{ textAlign: 'center', transform: [{ scale: -1 }] }} |
There was a problem hiding this comment.
The transform/scale here is a neat trick.
It should be encapsulated inside IconAllMessages. That also makes a good place for a code comment with the information from this commit message:
Web Zulip's all-messages icon is an inverted list icon, so we're
swapping mobile's all-messages icon to match.
It's fine if that means IconAllMessages has a slightly longer function definition than the others in Icons.js.
dootMaster
force-pushed
the
pr-homescreen-icon-change-5303
branch
from
0049773
to
441b6d9
Compare
|
@gnprice Sorry about that, I've pushed a new commit that should work now, although I don't know how to encapsulate the transform/scale prop into the function definition in |
|
Thanks for the revision. This pair of issues is still present: The quickest way to see them is to use
OK. Maybe the best approach there, then, is that when the PR is otherwise ready to merge, I'll add a commit on top that demonstrates the structure I have in mind, and merge with that. |
dootMaster
force-pushed
the
pr-homescreen-icon-change-5303
branch
2 times, most recently
from
4561d06
to
f33b6b3
Compare
to prepare the component for icon swapping.
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
force-pushed
the
pr-homescreen-icon-change-5303
branch
from
f33b6b3
to
eaf8e09
Compare
|
The previous commit should be fixed now. 🤦♂️ |
The current all-messages icon on mobile is a globe:
which doesn't match web Zulip's icon:
We're swapping it to the list icon for parity between mobile and web:
Fixes: #5303