-
Notifications
You must be signed in to change notification settings - Fork 368
icons: Add matchTextDirection to directional icons #2015
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
base: main
Are you sure you want to change the base?
Conversation
|
(Oops, I didn't mean to close this—accidentally clicked the wrong button.) |
|
Thanks, this looks great! This looks like a good approach, except I would maybe make a data file for the names of the directional icons. I've pushed two commits with some suggested changes, including that one, so here is the new version of your PR branch: 34d82b0 icons: Add matchTextDirection to directional icons Please pull those changes, and if they look good, please squash them into your commit and push the result to the PR. |
|
Ah, and also: there are some other icons with arrows that should probably get the same treatment:
And others that aren't arrows but also look like they should flip because they represent text in a directional layout:
|
|
Could you also add to the instructional comment "To add a new icon […]" in lib/widgets/icons.dart that developers should consider whether the icon is directional, and update that data if so? There's a Material doc you can link to to help the developer make the decision. |
ede8007 to
a66a0d1
Compare
|
Thanks for the refactor commits, @chrisbobbe! I've pulled them in and squashed everything into a single commit. I also updated directional_icons.json to include message_feed, topics, and arrow_left_right. I am a bit unsure about arrow_left_right, though—since it depicts a concept (two-way exchange) rather than a specific UI direction, I wonder if it usually stays static? I’ve included it for now to be safe, but I'm happy to remove it if you prefer. |
a66a0d1 to
8f3279e
Compare
|
Thanks! LGTM; marking for Greg's review. |
gnprice
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @yash-agarwa-l and @chrisbobbe! Comments below.
I think a good solution for both of these comments would be to treat this file as JS, not JSON: just rename it to a .js name, and use require instead of fs.readFileSync. (The file can still consist of just an array of strings.) Then take the opportunity to add code comments.
tools/icons/build-icon-font.js
Outdated
|
|
||
| // Icons that should flip horizontally in RTL layout. | ||
| const directionalIconsFile = path.join(srcDir, 'directional_icons.json'); | ||
| const directionalIcons = fs.readFileSync(directionalIconsFile); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Huh, does this decode the JSON?
… I think it doesn't, and in fact that has a visible effect: the icon topic gets mirrored, even though it's not in the list, because topics is in the list and its name contains the name "topic". The .includes call below is checking for string inclusion.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oop, thanks for the catch!
assets/icons/directional_icons.json
Outdated
| [ | ||
| "arrow_right", | ||
| "chevron_right", | ||
| "send", | ||
| "arrow_left_right", | ||
| "message_feed", | ||
| "topics" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think JSON is actually not a good choice for how to encode this. In general, JSON is a bad format to use for any sort of configuration file, or anything else that's going to be maintained over time by humans. (OTOH it's great for transferring data — the job it was designed for.)
The absolutely fatal problem with JSON as a config-file format is that it doesn't permit comments. Here, for example, I'd want to record the reasoning from #2015 (comment) as to why these particular icons are included.
8f3279e to
5147ad5
Compare
Fixes zulip#1907 We update the icon generation script to automatically set matchTextDirection: true for icons that imply directionality (arrow_right, chevron_right, send). This ensures these icons flip correctly in RTL locales. We also update the documentation within the generation script to instruct future contributors on how to handle new directional icons.
5147ad5 to
7796c01
Compare
|
Thanks! I've switched to a JS file and updated the import logic. |
This enables automatic mirroring for directional icons (like arrows and send buttons) in RTL languages.
Previously, icons like
chevron_rightandsendwould always point to the right, even in RTL layouts where the flow of the application is right-to-left. This updates the icon generation script to setmatchTextDirection: truefor these specific icons, ensuring they flip correctly.This addresses the fourth RTL issue identified by @gnprice in this comment, and fixes #1907.
Changes
tools/icons/build-icon-font.jsto addmatchTextDirection: trueforarrow_right,chevron_right, andsend.lib/widgets/icons.dartwith the regenerated code.Send Button