Skip to content
This repository has been archived by the owner on Aug 30, 2023. It is now read-only.

docs: add react i18n guide and organize MFE docs #83

Closed
wants to merge 2 commits into from

Conversation

bseverino
Copy link

@bseverino bseverino commented Mar 3, 2022

Description:

Issue: openedx/wg-frontend#37

Merge checklist:

  • All reviewers approved
  • CI build is green
  • Commits are squashed as needed
  • Review Read the Docs PR build to ensure changes appear as expected

@bseverino bseverino force-pushed the bseverino/react-i18n branch 2 times, most recently from 0e4166e to 5835315 Compare March 3, 2022 15:29
@bseverino bseverino force-pushed the bseverino/react-i18n branch from 5835315 to 9f22528 Compare March 10, 2022 21:31
Copy link

@ghassanmas ghassanmas left a comment

Choose a reason for hiding this comment

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

I tried to add few suggestions where I think it would make react_app_i18n.rst more appleaning to the general use, though you explicity state that those are edx specific requriement, (I guess it wouldn't hurt to make appealing given its a public doc) I leave to you to decide weather it make sesne to add them.

For jenkis pipelining stuff: Assumning someone from the community is creating a MFE to for the community to consume, would they need to go over those piblieing stuff? In other words is edx jenkis goning to be reposibsle for handeling all the jobs realted to the MFE in openedx and what if this new MFE is used heavily among the community but is irrelavant for U2/edX.

Not to say that I am experienced in edX/Jenkis but I just think having a context is needed for anyone to review.

Lastly is their a reason in the i18n steps you called it a React App, it's indeed a react app, but I though we always call it mircro-frontend, since also the steps starts by adding frontend-platform . I would suggest renaming react_app_i18n.rst. to something like i18n_of_micro_frontends but also to change all the reference in headeline...etc (if you agree with the suggestion)

host = https://www.transifex.com

[edx-platform.your-resource-name-here]
file_filter = src/i18n/messages/<lang>.json

Choose a reason for hiding this comment

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

So here [edx-platform.your-resource-name-here] Expectes the transfiex project name to be edx-platform, I think it would be worth to add a note about it, something like:

The line [edx-platform.your-resource-name-here] in the ./tx/config assumes your project name in transifex to be edx-platform, which is the case if you are building an app for the community to consume. However, if its for your own usage, you would have to change edx-platform with your own transfiex project name/slug.

****************
Set up Transifex
****************

Choose a reason for hiding this comment

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

To my knowlege this section might be required if:

  1. The developer is building/creating an app/MFE for the community consume.

  2. The developer is building an app/MFE for their private usage and they want to use transfiex as tool to translate their strings.

If nor 1 or 2 are ture, then this section can be skipped. So may be adding a note about it would make steps more clrear.

* Add ``reactifex`` to your dev dependencies::

npm install reactifex --save-dev

Choose a reason for hiding this comment

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

I think edx forked reactifex since its not maintained so the new alternative package might be @edx/reactifex, but double check that interanlly anyway.


# git repository
npm install @edx/frontend-component-header@git:https://github.com/edx/frontend-component-header-edx.git

Choose a reason for hiding this comment

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

I know this was wrriten before, but I guess this can good opportunity to change it.

The above example of installing using git is incorrect, it can be one of these:

npm install @edx/frontend-component-header@git+https://github.com/edx/frontend-component-header-edx.git
Or
npm install @edx/frontend-component-header@git://github.com/edx/frontend-component-header-edx.git
ref: https://docs.npmjs.com/cli/v8/commands/npm-install

@bseverino bseverino force-pushed the bseverino/react-i18n branch from 49a299b to b23b4c3 Compare April 6, 2022 20:22
@feanil
Copy link
Contributor

feanil commented Nov 15, 2022

Closing this since it hasn't had any changes in a while and this repo is now deprecated. If you're interested in moving these to the new docs site, This is the section that this probably goes in: https://github.com/openedx/docs.openedx.org/tree/main/source/translators

Deprecation issue: #112

@feanil feanil closed this Nov 15, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants