-
Notifications
You must be signed in to change notification settings - Fork 6
[DIT-12000] Add iosLocales Configuration Support #132
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: string-file-formats
Are you sure you want to change the base?
[DIT-12000] Add iosLocales Configuration Support #132
Conversation
laurakoye
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.
I didn't encounter any issues testing. I know you wanted to get another review, so I'll just leave my comments.
| const swiftDriverFile = await this.getSwiftDriverFile(); | ||
| files.push(swiftDriverFile); | ||
| } | ||
| await super.writeFiles([...files]); |
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.
You don't need the spread here
| } catch (e) { | ||
| if (!(e instanceof AxiosError)) { | ||
| throw new Error( | ||
| "Sorry! We're having trouble reaching the Ditto API. Please try again later." | ||
| ); | ||
| } | ||
|
|
||
| throw e; |
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.
nit: You could add some more specific errors based on the http status of the response below?
| describe("getLocalesPath", () => { | ||
| it("should return output outDir when iosLocales is not configured", () => { | ||
| const projectConfig = createMockProjectConfig({ | ||
| iosLocales: undefined, |
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.
Add test for iosLocales: []
| // map "base" to undefined, as by default export endpoint returns base variant | ||
| const variantsParam = | ||
| variant.id === "base" ? undefined : [{ id: variant.id }]; | ||
| const variantId = variant.id === "base" ? undefined : variant.id; |
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.
In general we could extract some commonly used constants into a constants file?
export const BASE_VARIANT_ID = "base";
export const SWIFT_DRIVER_FILENAME = "Ditto";
export const FILE_NAME_SEPARATOR = "___";
export const COMPONENTS_FILE_PREFIX = "components";
| /*********************************************************** | ||
| * getLocalesPath | ||
| ***********************************************************/ |
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 feel like this comment is unnecessary if we have the describe block
| @@ -0,0 +1,16 @@ | |||
| import OutputFile from "./OutputFile"; | |||
|
|
|||
| export default class SwiftOutputFile extends OutputFile<string, {}> { | |||
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.
Do we want the file name to match the class name?
|
|
||
| protected async writeFiles(files: OutputFile[]): Promise<void> { | ||
| if (this.projectConfig.iosLocales) { | ||
| const swiftDriverFile = await this.getSwiftDriverFile(); |
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.
If we call writeFiles for both iosStringsDict and iosStrings. Does that mean we are generating the driver file twice?
| excludeNestedFolders: z.boolean().optional(), | ||
| })).optional(), | ||
| }).optional(), | ||
| components: z |
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.
Is this just a format diff? I don't see what changed?
| "start": "node esbuild.mjs && node --enable-source-maps --require dotenv/config bin/ditto.js", | ||
| "sync": "node esbuild.mjs && node --enable-source-maps --require dotenv/config bin/ditto.js pull" | ||
| "sync": "node esbuild.mjs && node --enable-source-maps --require dotenv/config bin/ditto.js pull", | ||
| "sync-legacy": "node esbuild.mjs && node --enable-source-maps --require dotenv/config bin/ditto.js --legacy pull" |
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.
Why do we need this?
| variants: z.array(z.object({ id: z.string() })).optional(), | ||
| outDir: z.string().optional(), | ||
| richText: z.union([z.literal("html"), z.literal(false)]).optional(), | ||
| iosLocales: z.array(z.record(z.string(), z.string())).optional(), |
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.
If we care about validating inputs more, we could use .refine and we check if all the variants in iosLocales are in the variants array.
laurakoye
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.
Found what I think is an issue:
https://www.loom.com/share/d619af17b17d4f7f95991f894ed2600c
If I have a variant applied to a text item the generated string and string dics are showing the base text. This is true even if the variant has plurals on it.
Overview
iosLocalesto project YAML configiosLocalesprovided in conjunction withios-stringsorios-stringsdictoutputs, will write mapped variants to a<locale>.projdirectory in the project YAML'soutDiriosLocales, will write tooutput'soutDiras usualContext
DIT-12000: [CLI] Add iOSLocales Configuration
Test Plan
Config setup: Make sure to have
iosLocalesat the root of your project directory. Ideally you have a few variants in your local setup. If so, have at least a couple of them mapped to a locale, e.g.base: en,spanish: esThis setup allowed me to ensure that spanish and english files were written to the project's outDir and not the individual
output'soutDiryarn start pull, english and spanish variant files for both.stringsand.stringsdictshould be written to anen.lprojandes.lprojdirectory at the root of the project'soutDirDitto.swiftfile should be written to the root of the project'soutDir. It should have actual swift code in it loliosLocalesshould write to theoutput'soutDiras expected