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

Add context to provide rtl support for react-icons #560

Open
wants to merge 43 commits into
base: main
Choose a base branch
from

Conversation

tomi-msft
Copy link
Contributor

This PR is to add a react context to the icon build that will allow icons to flip in rtl. This is the first step of the process. The next step is to create/update a list of flippable icons to apply the rtl styling to. Part of #553

tomi-msft and others added 30 commits November 23, 2021 13:33
@tomi-msft tomi-msft requested a review from behowell March 17, 2023 20:54
@@ -16,7 +16,7 @@
"cleanSvg": "rm -rf ./intermediate",
"copy": "node ../../importer/generate.js --source=../../assets --dest=./intermediate --extension=svg --target=react",
"copy:font-files": "cpy './src/utils/fonts/*.{ttf,woff,woff2,json}' ./lib/utils/fonts/. && cpy './src/utils/fonts/*.{ttf,woff,woff2,json}' ./lib-cjs/utils/fonts/.",
"convert:svg": "node convert.js --source=./intermediate --dest=./src",
"convert:svg": "node convert.js --source=./intermediate --dest=./src --filter=../../importer/rtl.txt",
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't see rtl.txt included in this PR? If there is an existing file, it might be used for a different purpose? We probably want to create a new file for this feature.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The rtl.txt file is here.

I am wary about creating a new file for this because this file was already created by the designers to track icons that should be flipped in rtl

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need to track down with the Android folks (that's the only platform previously using the rtl.txt file) what is/isn't there. RTL wasn't kept up with, for the most part, as other platforms didn't need/use it.
We should also collaborate with the Apple folks on that, as they are the platform that has the most RTL/LTR support in, and it's supported by the platform itself, but we need to make sure everything has equivalency.

@@ -10,6 +10,7 @@ const _ = require("lodash");

const SRC_PATH = argv.source;
const DEST_PATH = argv.dest;
const RTL_FILTER_PATH =argv.filter;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

filter is a confusing name for this arg. How about flip_in_rtl?

Ditto for the file name: flip_in_rtl.txt instead of rtl.txt

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Possibly even autoflip_in_rtl

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants