feat: Added scripts and marked strings for i18n. #396
Conversation
saleem-latif
requested review from
adamstankiewicz,
muhammad-ammar,
irfanuddinahmad and
jajjibhai008
saleem-latif
force-pushed
the
saleem-latif/ENT-8580
branch
3 times, most recently
from
4e36443 to
cb2edf1
Compare
| } from './data/constants'; | ||
| import { | ||
| useActiveRefinementsAsFlatArray, | ||
| } from './data/hooks'; | ||
| import { SearchContext } from './SearchContext'; | ||
| import { removeFromRefinementArray, deleteRefinementAction } from './data/actions'; | ||
|
|
||
| const messages = defineMessages({ | ||
| [LEARNING_TYPE_COURSE]: { | ||
| id: 'search.facetFilters.filterTitle.course', |
There was a problem hiding this comment.
We are not following Camel case format in IDs of strings. So I think, we can maintain the consistency as we are doing in learner portal and admin portal.
search.facetFilters.filterTitle.course -> search.facet.filters.filter.title.course
There was a problem hiding this comment.
I think it is better to use dot . to separate pages/components and use camelCase for page/component name.
There was a problem hiding this comment.
Agreed, camelCase is preferred for message ids.
| description: 'Title for the course filter.', | ||
| }, | ||
| [LEARNING_TYPE_PROGRAM]: { | ||
| id: 'search.facetFilters.filterTitle.program', |
There was a problem hiding this comment.
Same as mentioned above about id format
| description: 'Title for the program filter.', | ||
| }, | ||
| [LEARNING_TYPE_PATHWAY]: { | ||
| id: 'search.facetFilters.filterTitle.pathway', |
There was a problem hiding this comment.
Same as mentioned above about id format. And please do it on otherwise her places too.
saleem-latif
force-pushed
the
saleem-latif/ENT-8580
branch
from
cb2edf1 to
59ce78b
Compare
| } from './data/constants'; | ||
| import { | ||
| useActiveRefinementsAsFlatArray, | ||
| } from './data/hooks'; | ||
| import { SearchContext } from './SearchContext'; | ||
| import { removeFromRefinementArray, deleteRefinementAction } from './data/actions'; | ||
|
|
||
| const messages = defineMessages({ | ||
| [LEARNING_TYPE_COURSE]: { | ||
| id: 'search.facetFilters.filterTitle.course', |
There was a problem hiding this comment.
Agreed, camelCase is preferred for message ids.
| @@ -42,6 +43,10 @@ const SearchData = ({ children, searchFacetFilters, trackingName }) => { | |||
| refinementsReducer, | |||
| {}, | |||
| ); | |||
| const intl = useIntl(); | |||
| const searchFacetFiltersWithI18n = ( | |||
| searchFacetFilters === null ? getSearchFacetFilters(intl) : searchFacetFilters | |||
There was a problem hiding this comment.
nit: might make sense to check for falsey (i.e., not explicitly checking for null). E.g., if someone were to pass searchFacetFilters={undefined} as the prop.
Might also make sense to reverse the ordering of your conditional.
const searchFacetFiltersWithI18n = searchFacetFilters || getSearchFacetFilters(intl);nit x2: If searchFacetFilters is passed as a prop, there's not actually a guarantee the search filters are using i18n messages; they could be passed with hardcoded English strings (as Learner Portal is passing today). Is it slightly misleading to have this variable named searchFacetFiltersWithI18n when if custom searchFacetFilters are provided, they may not actually use i18n?
| id: 'catalog.search.pagination.page.of.count', | ||
| defaultMessage: 'of', | ||
| description: 'Label for the page of count in the pagination component', | ||
| }), |
There was a problem hiding this comment.
[clarification] Is the intent of this PR to migrate all hardcoded English strings? If so, should we also be migrating the following hardcoded English strings?
saleem-latif
force-pushed
the
saleem-latif/ENT-8580
branch
from
59ce78b to
af685c9
Compare
|
@adamstankiewicz I have made changes according to your feedback, please take another look. |
Jira Ticket: ENT-8580
Description:
This PR adds i18n support for
catalog-searchpackage.Merge checklist:
frontend-app-learner-portal-enterprise,frontend-app-admin-portal, andfrontend-app-enterprise-public-catalog). Will consumers safely be able to upgrade to this change without any breaking changes?BREAKING CHANGEso the NPM package is released as such.Post merge:
chore(release): publish new versions) that incremented versions in relevant package.json and CHANGELOG files, and created Git tags for those versions is onmaster(Important: ensure the Git tags are for the correct commit SHA).Publish from package.jsonGithub Action workflow to publish these new package versions to NPM.masterbranch.npm view <package_name> versions --json).