Adds clarifications to README for new fbtee users #20

tsteckenborn · 2025-01-04T11:07:03Z

This pull request updates the README to clarify a few common stumbling blocks for developers who are new to fbtee. The changes provide additional context on a few commands, how they are used, and some pitfalls or nuances that might not be obvious to first-time users.

…ocks

kayhadrin · 2025-01-04T16:07:17Z

@tsteckenborn, thanks for helping us improve the fbtee documentation!
I appreciate you taking the time to describe the role of the 3 main fbtee CLI commands to new users!

Here are some comments to help refine this PR before merging it. :-)

tsteckenborn · 2025-01-04T16:41:58Z

Here are some comments to help refine this PR before merging it. :-)

Thanks for the prompt reaction. I assume the comments are not yet published (atleast not seeing review comments yet)?

kayhadrin · 2025-01-04T12:39:46Z

README.md

@@ -38,25 +44,31 @@ export default {
 };
 ```

-**fbtee** uses three scripts to manage translations. These scripts help automate the process of collecting, creating, and compiling translations. It is recommended to add them to your `package.json`:
+**fbtee** uses three scripts to manage translations. These scripts help automate the process of collecting, creating, and compiling translations. Dependending on your setup you might adjust want to adjust them, which we'll cover below. It is recommended to add them to your `package.json`:


Suggestion:

... These scripts help automate the process of collecting, creating, and compiling translations, which we'll cover below. Dependending on...

Ah, my meaning of cover below isn't matching on how you read that. So I might need to improve that text. I didn't want to say that we cover the collection, creating and compiling below, but the adjustment of the setup / commands. Never the less there's an issue with two times basically the same thing in there.

I see what you mean now, but I was mainly trying to fix the phrasing of this sentence:

Dependending on your setup you might adjust want to adjust them, which we'll cover below.

I also noticed this mispeled word... "Dependending" 😅

README.md

kayhadrin · 2025-01-04T14:47:44Z

README.md

@@ -65,6 +77,8 @@ The files generated by these commands are auto-generated and should not be check
 src/translations.json


Normally, for best practice, I'd recommend storing the translations file in the repo alongside the source code because website owners should always be able figure out what exact translations were being used on the website at any point in time.
Typical scenario for legal investigations: "How were the site's T&Cs in Spanish last year when we were promoting this product?"
Of course, YMMV depending on how developers build their websites.

Did this comment slip to another point? If I'm not mistaken that line was already in there. Yet if I understood the setup that was aimed for correctly, then this wouldn't be required as in theory it's anyways always build from the individual translation files that are checked in?

kayhadrin · 2025-01-04T14:50:56Z

README.md

@@ -82,13 +96,19 @@ setupFbtee({
 });
 ```

-Finally, if you are using React and TypeScript in your project, you need to add TypeScript types for **fbtee** to enable proper type checking in JSX. You can do this by referencing the `ReactTypes.d.ts` file in your main `index.tsx` file or a global type declaration file (e.g., `types.d.ts`):
+> [!NOTE]
+> At this point you've not yet generated the translation file. Therefore you'll see an error for it no tbeing able to find the module `./translations.json`. This is expected and will be resolved once you've generated the translation file. Simply running the command at this point wouldn't remedy it, as the empty json file wouldn't adhere to the expected type and would also throw an error.


We wouldn't need to write this warning if we moved the fbtee clienside setup instructions later, after explaining how to assemble translation files,

That is correct. However, in my view, it might make sense to organise the entire ReadMe slightly differently. It might make sense to start with the explanation of usage within the codebase, as the commands themselves are only really ‘usable’ once some translations are in the codebase.

kayhadrin · 2025-01-04T14:52:33Z

README.md

+> At this point you've not yet generated the translation file. Therefore you'll see an error for it no tbeing able to find the module `./translations.json`. This is expected and will be resolved once you've generated the translation file. Simply running the command at this point wouldn't remedy it, as the empty json file wouldn't adhere to the expected type and would also throw an error.
+
+> [!WARNING]
+> To be described: How to handle e.g. React Router with "framework mode" with a client and server entry. How to define the language based on e.g. the users browser settings and ensure that there are no hydration errors or the like. Currently the sample is a hardcoded locale and gender.


This WARNING note would be more relevant as a new TODO item to add to #11 instead. We can do that later outside of this PR I think.

I agree. Do you add it to the issue, I'll remove it here?

kayhadrin · 2025-01-04T15:49:15Z

README.md

+Let's go through these commands ones by one:
+
+- `fbtee manifest --src src` searches through files in the `src` directory and generates a manifest file (`.src_manifest.json`) that lists all the files containing `<fbt>` tags. In addition it creates `.enum_manifest.json` which lists all the files containing `<fbt:enum>` tags. These files will be put into `.gitignore` as they are auto-generated and should not be checked into version control. You can adjust the `--src` parameter to point to your source directory. If your source directory is for example the root use `--src .` or if it's an app folder use `--src app`.
+- `fbtee collect --manifest < .src_manifest.json > .source_strings.json` reads the manifest file and extracts all the strings marked for translation into a `.source_strings.json` file. This file should be uploaded to your translation provider as the source for your translations. The file itself will also be put into `.gitignore`. As you might be able to see in theory both the output file name of the manifest command, as well as input and ouput of the collect command are configurable. Yet we recommend to stick with the default values.


Quote:

The file itself will also be put into .gitignore.

Ditto from above comment.

As you might be able to see in theory both the output file name of the manifest command, as well as input and ouput of the collect command are configurable. Yet we recommend to stick with the default values.

This seems self-explanatory and I think this sentence could be removed for conciseness.

I generally agree, but since many configurations can be ‘copied and pasted’, I wanted to point out a sort of ‘beware, you may need to change this’.

kayhadrin · 2025-01-04T15:58:47Z

README.md

+
+- `fbtee manifest --src src` searches through files in the `src` directory and generates a manifest file (`.src_manifest.json`) that lists all the files containing `<fbt>` tags. In addition it creates `.enum_manifest.json` which lists all the files containing `<fbt:enum>` tags. These files will be put into `.gitignore` as they are auto-generated and should not be checked into version control. You can adjust the `--src` parameter to point to your source directory. If your source directory is for example the root use `--src .` or if it's an app folder use `--src app`.
+- `fbtee collect --manifest < .src_manifest.json > .source_strings.json` reads the manifest file and extracts all the strings marked for translation into a `.source_strings.json` file. This file should be uploaded to your translation provider as the source for your translations. The file itself will also be put into `.gitignore`. As you might be able to see in theory both the output file name of the manifest command, as well as input and ouput of the collect command are configurable. Yet we recommend to stick with the default values.
+- `fbtee translate --translations translations/*.json --jenkins > src/translations.json` reads all the translations in the `translations/` directory and compiles them into a single `src/translations.json` file. This file is used by **fbtee** to display translated content in your app. The `--translations` parameter specifies the path to the translation files. The `--jenkins` flag is used to define the utilized [hash function](https://en.wikipedia.org/wiki/Jenkins_hash_function). You can adjust the `--translations` parameter to point to your translation directory. If your translations are within your app directory in a folder called `i18n`, for example, you'd use `--translations app/i18n/*.json`. The output is the translation file used by **fbtee**. This file is going to be referred to in your applications entry point, therefor it needs to be generated as part of the build, but should not be part of your version control. Also here you should adjust the output file name to your needs. For example if we're going with an app folder structure we could use `> app/translations.json`. As this command requires the translations to be present in the `translations/` directory to create a valid json to be used, you first need to create such files based on the `.source_strings.json` received as part of the collect command.


Suggesting some rewording to simplify the definition of the role of translations.json

In my opinion, I don't think we should discourage users not to version their translation files. Of course, they can ignore that if they already have other ways to version + archive their translation builds.

I think we can assume that users would already know how to customize file paths for their own projects.

Before

The output is the translation file used by fbtee. This file is going to be referred to in your applications entry point, therefor it needs to be generated as part of the build, but should not be part of your version control. Also here you should adjust the output file name to your needs. For example if we're going with an app folder structure we could use > app/translations.json.

After:

The script will store translations for all the provided locales into a single file src/translations.json destined to be used by fbtee on the clientside. This file contents will later be provided to the fbtee library during the client-side setup phase (See setupFbtee), therefore it needs to be generated & bundled as part of your website build.

On 1) With regards to the proposed after: Given this formulation one could raise the question on what's then used on server side, given that the meaning of client and server in this context, and the context of the framework I'm working in might not be identical.

Maybe whoever wrote the original ReadMe could chime in given it was already marked as to be put into .gitignore. In general I assume it's a duplication, if the translate command would be part of the build. If it's not, then I agree.

As commented above a lot of instructions that atleast I'm used to can be simply copied. It shouldn't harm to put attention that this is not the case, but that already here some assumptions are being made (e.g. your code being in a src repository, some other files being in root). Yet up to you all.

kayhadrin · 2025-01-04T16:00:01Z

README.md

 ```

-The files generated by these commands are auto-generated and should not be checked into version control. Add the following entries to your `.gitignore`:
+The files generated by these - as previously said - should not be checked into version control. Add the following entries to your `.gitignore`:


Before

The files generated by these - as previously said - should not be checked into version control. Add the following entries to your .gitignore:

After

The files generated by these commands - as previously said - should not be checked into version control. Add the following entries to your .gitignore:

kayhadrin · 2025-01-04T16:00:18Z

README.md

@@ -65,6 +77,8 @@ The files generated by these commands are auto-generated and should not be check
 src/translations.json
 ```

+Be sure to adjust the paths to your needs if you're not using the default values.


> [!NOTE] > Be sure to adjust the paths to your needs if you're not using the default values.

kayhadrin · 2025-01-04T16:02:08Z

README.md

@@ -82,13 +96,19 @@ setupFbtee({
 });
 ```

-Finally, if you are using React and TypeScript in your project, you need to add TypeScript types for **fbtee** to enable proper type checking in JSX. You can do this by referencing the `ReactTypes.d.ts` file in your main `index.tsx` file or a global type declaration file (e.g., `types.d.ts`):
+> [!NOTE]
+> At this point you've not yet generated the translation file. Therefore you'll see an error for it no tbeing able to find the module `./translations.json`. This is expected and will be resolved once you've generated the translation file. Simply running the command at this point wouldn't remedy it, as the empty json file wouldn't adhere to the expected type and would also throw an error.


Minor typos if you're keeping this text:

- ... Therefore you'll see an error for it no tbeing able to find the module `./translations.json`. + ... Therefore, you'll see an error for it not being able to find the module `./translations.json`.

tsteckenborn · 2025-01-04T18:04:48Z

@kayhadrin commented on the comments. Didn't yet adjust anything, as we probably anyways need to do a full rereview when we'd decide to change the structure in general. Could probably rework it on monday or tuesday, depending on other tasks.

tsteckenborn · 2025-01-07T19:59:04Z

@kayhadrin shall I create a version that changes the general structure as an alternate proposal?

cpojer · 2025-01-08T02:43:31Z

Thank you so much for updating the README. In the interest of moving fast, I did a pass on your changes to make it mergeable right now while we still want to make many changes to the library and the README. Your updates were eye-opening as part of what an early adopter might experience (like the note that the translations.json file is not going to work at first). I dropped that from the README, and we should just fix that in the library itself.

Also, I'm actually thinking of getting rid of the single json file as it doesn't scale and make the default be a folder with each translation being separate.

docs: extends the current documentation to explain a few stumbling bl…

0e91b1d

…ocks

kayhadrin added the documentation Improvements or additions to documentation label Jan 4, 2025

kayhadrin assigned tsteckenborn Jan 4, 2025

kayhadrin self-requested a review January 4, 2025 16:08

kayhadrin requested changes Jan 4, 2025

View reviewed changes

Iterate.

037eeeb

cpojer merged commit bebe599 into nkzw-tech:main Jan 8, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adds clarifications to README for new fbtee users #20

Adds clarifications to README for new fbtee users #20

tsteckenborn commented Jan 4, 2025

kayhadrin commented Jan 4, 2025 •

edited

Loading

tsteckenborn commented Jan 4, 2025

kayhadrin Jan 4, 2025

tsteckenborn Jan 4, 2025

kayhadrin Jan 5, 2025 •

edited

Loading

kayhadrin Jan 4, 2025

tsteckenborn Jan 4, 2025

kayhadrin Jan 4, 2025

tsteckenborn Jan 4, 2025

kayhadrin Jan 4, 2025

tsteckenborn Jan 4, 2025

kayhadrin Jan 4, 2025 •

edited

Loading

tsteckenborn Jan 4, 2025

kayhadrin Jan 4, 2025 •

edited

Loading

tsteckenborn Jan 4, 2025

kayhadrin Jan 4, 2025

kayhadrin Jan 4, 2025

kayhadrin Jan 4, 2025

tsteckenborn commented Jan 4, 2025

tsteckenborn commented Jan 7, 2025

cpojer commented Jan 8, 2025

		@@ -65,6 +77,8 @@ The files generated by these commands are auto-generated and should not be check
		src/translations.json

Adds clarifications to README for new fbtee users #20

Adds clarifications to README for new fbtee users #20

Conversation

tsteckenborn commented Jan 4, 2025

kayhadrin commented Jan 4, 2025 • edited Loading

tsteckenborn commented Jan 4, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kayhadrin Jan 5, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kayhadrin Jan 4, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kayhadrin Jan 4, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tsteckenborn commented Jan 4, 2025

tsteckenborn commented Jan 7, 2025

cpojer commented Jan 8, 2025

kayhadrin commented Jan 4, 2025 •

edited

Loading

kayhadrin Jan 5, 2025 •

edited

Loading

kayhadrin Jan 4, 2025 •

edited

Loading

kayhadrin Jan 4, 2025 •

edited

Loading