-
Notifications
You must be signed in to change notification settings - Fork 69
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
[BD-46] feat: implement i18n in Paragon components and in docs site #1100
[BD-46] feat: implement i18n in Paragon components and in docs site #1100
Conversation
✅ Deploy Preview for paragon-openedx ready!Built without sensitive environment variables
To edit notification comments on pull requests, go to your Netlify site settings. |
✔️ Deploy Preview for paragon-edx ready! 🔨 Explore the source changes: 08ebdef 🔍 Inspect the deploy log: https://app.netlify.com/sites/paragon-edx/deploys/623058c031fb4e00087735a5 😎 Browse the preview: https://deploy-preview-1100--paragon-edx.netlify.app |
Thanks for the pull request, @viktorrusakov! When this pull request is ready, tag your edX technical lead. |
src/Card/index.jsx
Outdated
@@ -24,7 +24,7 @@ const Card = React.forwardRef(({ | |||
clickable: isClickable, | |||
})} | |||
ref={ref} | |||
tabindex={isClickable ? '0' : '-1'} |
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.
[inform] This was fixed in a separate PR and should be all set upon a rebase of this branch.
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.
Yeah, rebasing removed it, thanks!
src/DataTable/DataTableLayout.jsx
Outdated
}; | ||
|
||
DataTableLayout.propTypes = { | ||
className: PropTypes.string, | ||
children: PropTypes.node.isRequired, | ||
filtersTitle: PropTypes.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.
This prop should likely accept things beyond strings (either node or element maybe?) , e.g. say a consumer wants to wrap the title in a custom element with a class name, (e.g., filtersTitle={<span className="...">{...}</span>}
).
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.
Yeah, it definitely should, missed it, thanks
src/DataTable/FilterStatus.jsx
Outdated
{clearFiltersText || ( | ||
<FormattedMessage | ||
id="pgn.DataTable.FilterStatus.clearFiltersText" | ||
defaultMessage="Clear Filters" |
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.
Lowercase "filters"
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.
Updated, thanks
src/DataTable/ExpandAll.jsx
Outdated
@@ -1,12 +1,29 @@ | |||
import React from 'react'; | |||
import PropTypes from 'prop-types'; | |||
import { FormattedMessage } from 'react-intl'; | |||
import { Button } from '..'; | |||
|
|||
const ExpandAll = ({ getToggleAllRowsExpandedProps, isAllRowsExpanded }) => ( | |||
<span {...getToggleAllRowsExpandedProps()}> | |||
{isAllRowsExpanded |
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.
formatting nit:
{isAllRowsExpanded ? (
<Button variant="link" size="inline">
<FormattedMessage
id="pgn.DataTable.ExpandAll.collapseAllLabel"
defaultMessage="Collapse all"
description="Title of the filters components"
/>
</Button>
) : (
<Button variant="link" size="inline">
<FormattedMessage
id="pgn.DataTable.ExpandAll.expandAllLabel"
defaultMessage="Expand all"
description="Title of the filters components"
/>
</Button>
)}
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.
yeah, this looks better, thanks 🙂
className: PropTypes.string, | ||
/** A text that appears on the `Clear selection` button, defaults to 'Clear Selection' */ | ||
clearSelectionText: PropTypes.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.
Should this also handle elements in addition to strings as well?
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.
it should, thanks, updated
floatingLabel="Direction" | ||
> | ||
<option value="ltr">Left to right</option> | ||
<option value="rtl">Right to left</option> | ||
</Form.Control> | ||
</Form.Group> | ||
)} | ||
{FEATURES.LANGUAGE_SWITCHER && ( |
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.
Let's maybe use Stack
here to add a bit of vertical space between these 3 Form.Control
components (e.g., <Stack gap={5}>
) ?
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.
Good idea! Added Stack gap={3}
, setting gap to 5
seemed like too much extra space
document.body.setAttribute('dir', value); | ||
} | ||
setSettings(prevState => ({ ...prevState, [key]: value })); | ||
global.localStorage.setItem('pgn__settings', JSON.stringify({ ...settings, [key]: value })); |
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.
Nice, this is a great idea to colocate the settings to a single storage item!
ba82c89
to
08ebdef
Compare
Codecov Report
@@ Coverage Diff @@
## master #1100 +/- ##
=======================================
Coverage 90.87% 90.87%
=======================================
Files 198 199 +1
Lines 3266 3288 +22
Branches 735 753 +18
=======================================
+ Hits 2968 2988 +20
- Misses 284 286 +2
Partials 14 14
Continue to review full report at Codecov.
|
08ebdef
to
61e880b
Compare
[main] | ||
host = https://www.transifex.com | ||
|
||
[edx-platform.paragon] |
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.
Should this be a separate project, not edx-platform
?
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.
As discussed, we will keep this under edx-platform
for the time being, as it seems most of the projects that consume Paragon are under this project as well. I agree it's a bit odd, though.
bf1ef73
to
f14c771
Compare
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.
Awesome! Left a few nits here and there, but this is looking great 🎉
[main] | ||
host = https://www.transifex.com | ||
|
||
[edx-platform.paragon] |
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.
As discussed, we will keep this under edx-platform
for the time being, as it seems most of the projects that consume Paragon are under this project as well. I agree it's a bit odd, though.
.tx/config
Outdated
file_filter = src/i18n/messages/<lang>.json | ||
source_file = src/i18n/transifex_input.json | ||
source_lang = en | ||
type = STRUCTURED_JSON |
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: newline
Makefile
Outdated
i18n = ./src/i18n | ||
transifex_input = $(i18n)/transifex_input.json | ||
|
||
NPM_TESTS=build i18n_extract lint test is-es5 |
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 think the is-es5
script can be removed, as Paragon does not have it defined?
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.
true, thanks!
Makefile
Outdated
# This target is used by Travis. | ||
validate-no-uncommitted-package-lock-changes: | ||
# Checking for package-lock.json changes... | ||
git diff --exit-code package-lock.json |
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: newline
README.md
Outdated
) | ||
``` | ||
|
||
Note that if you are using ``frontend-platform``'s ``AppProvider`` component you don't need a separate context, |
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: @edx/frontend-platform
@@ -56,10 +56,10 @@ Consequences | |||
ReactDOM.render( | |||
<AppProvider> | |||
<App /> | |||
</IntlProvider>, | |||
</AppProvider>, |
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.
D'oh ;)
package.json
Outdated
@@ -47,6 +48,7 @@ | |||
"prop-types": "^15.8.1", | |||
"react-bootstrap": "^1.6.4", | |||
"react-focus-on": "^3.5.4", | |||
"react-intl": "2.9.0", |
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.
Thanks for keeping this pinned 🙌
it('hides filter text', () => { | ||
const wrapper = mount(<FilterStatusWrapper value={instance} props={filterPropsNoFiltered} />); | ||
expect(wrapper.text()).toEqual(''); | ||
}); |
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.
Should we be removing this test? Is there another test covering the showFilteredFields={false}
case? There is at least one or two teams using this prop to hide the "Filtered by..." text because it shows the accessor
's value which may be a programmatic value, e.g. "Filtered by requestStatus".
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.
Yeah, my bad...fixed it and reverted the change, glad you noticed, thanks!
www/src/components/PropsTable.jsx
Outdated
@@ -69,7 +69,7 @@ const PropsTable = ({ props: componentProps, displayName, content }) => ( | |||
{content && <div className="small mb-3">{content}</div>} | |||
{componentProps.length > 0 ? ( | |||
<ul className="list-unstyled"> | |||
{componentProps.map(metadata => <Prop key={metadata.name} {...metadata} />)} | |||
{componentProps.map(metadata => metadata.name !== 'intl' && <Prop key={metadata.name} {...metadata} />)} |
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: think it's worth creating a IGNORED_COMPONENT_PROPS=['intl']
(or similar) and use it in a .filter(...)
to make this simple to add to in the future if needed?
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.
This definitely seems like a better approach, thanks 🙂
@viktorrusakov Is there any way we'd be able to support a case where a code repository is using Related: One idea worth exploring here is whether we prioritize getting |
f14c771
to
c607100
Compare
@adamstankiewicz I'm pretty positive that there will be issues if provider has As for updating |
@viktorrusakov Yeah, it might be worth a manual test, if possible. I'm also leaning towards prioritizing the upgrade to I just added some notes to the linked issue about the impacts of each breaking change in Let me know your thoughts after you give those release notes a review! Maybe we can chat about this in more detail during our "in-person" sync today. |
@viktorrusakov Yes, I agree we should do something here as well. I'm not sure if we will directly get involved with upgrading Transifex scripts in each project, though we could likely go that route, if we'd like. I might be leaning more towards creating the documentation / examples for it, and empowering teams to make the improvements themselves. Open to your thoughts here on methodology, too, of course 😄 |
@adamstankiewicz Tested Paragon's i18n implementation with Judging from the breaking changes list for v3 release I think the reason might be
I don't know what this changes exactly 😅 but |
c607100
to
acc532e
Compare
6b1734e
to
02d3268
Compare
c259dce
to
cd58bdb
Compare
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.
Hey @viktorrusakov, I left a few comments throughout the PR. Two notable concerns:
- Should we treat
react-intl
as a peer dependency (see comment for more details)? - If the default message is from a variable defined as a constant (as in
Alert
), it doesn't appear that its string is exported in the exported transifex_input.json file, e.g.:
{
"pgn.Alert.closeLabel": {
"developer_comment": "Label of a close button on Alert component"
},
}
On another note, I'm working to get the PRs for the Transifex pipeline job created for the repo in our Jenkins instance as well along with alerting for when the jobs fail.
package.json
Outdated
@@ -55,6 +56,7 @@ | |||
"prop-types": "^15.8.1", | |||
"react-bootstrap": "^1.6.4", | |||
"react-focus-on": "^3.5.4", | |||
"react-intl": "^5.25.0", |
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.
Should we treat react-intl
as a peer dependency, especially since it's already included as a dependency in @edx/frontend-platform
? That way, @edx/frontend-platform
's version of react-intl
would remain the source of truth. It would also allow peer dependency warnings during install if the consumer doesn't have react-intl
(at the correct version) in their installed dependency tree.
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.
Agree, that's a great idea!
src/DataTable/ExpandAll.jsx
Outdated
<FormattedMessage | ||
id="pgn.DataTable.ExpandAll.expandAllLabel" | ||
defaultMessage="Expand all" | ||
description="Title of the filters components" |
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.
Should this description differ from the "Collapse all" message above?
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.
Yeah... and "Collapse all" message also should be different and not copied from some filters component 😅
updated, thanks!
@@ -23,7 +24,15 @@ const FilterStatus = ({ | |||
size={size} | |||
onClick={() => setAllFilters([])} | |||
> | |||
{clearFiltersText} | |||
{clearFiltersText === 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.
[curious] Why the explicit check for the undefined
type versus relying on a falsey value alone? For example, if consumer passes an empty string, I'd think we might still want to fallback to the default message?
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.
Good question! Initially I tried to check for falsey value but decided against it because there is a specific test case which tests that no text gets rendered if you pass an empty string, it was failing.
I could change that test but I thought that maybe there is a use case for not displaying any text..
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.
Hmm, interesting. I suppose that could be possible; best to handle it explicitly I guess, considering there is a specific test for it 🤷♂️ Though, I cant imagine why you'd want to do this haha.
<FormattedMessage | ||
id="pgn.DataTable.BaseSelectionStatus.selectAllText" | ||
defaultMessage="Select all {itemCount}" | ||
description="Clear selection button label" |
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.
Should this description reflect that it's the "Select all" message?
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.
Definitely, thanks!
const [autoHide, setAutoHide] = useState(true); | ||
const intlCloseLabel = closeLabel || intl.formatMessage({ | ||
id: 'pgn.Toast.closeLabel', | ||
defaultMessage: 'Close', |
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.
Should this be using the TOAST_CLOSE_LABEL_TEXT
constant?
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.
Maybe not, if the string
isn't getting extracted when this is treated as a constant 🙃
<Form.Group> | ||
<Form.Control | ||
as="select" | ||
value={currentTheme} | ||
onChange={onThemeChange} | ||
value={settings.theme} |
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.
Should the Form.Control
for the direction switcher also be reading from settings
?
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.
Yeah, good catch, thanks! Rebasing is hard 🙃
cd58bdb
to
fb7519d
Compare
fb7519d
to
18760fd
Compare
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.
LGTM 🚀 I have a PR created that is also awaiting review from our DevOps team to enable the Transifex pipeline jobs on Jenkins to automate the push/pull of translations for the repo.
@@ -23,7 +24,15 @@ const FilterStatus = ({ | |||
size={size} | |||
onClick={() => setAllFilters([])} | |||
> | |||
{clearFiltersText} | |||
{clearFiltersText === 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.
Hmm, interesting. I suppose that could be possible; best to handle it explicitly I guess, considering there is a specific test for it 🤷♂️ Though, I cant imagine why you'd want to do this haha.
@viktorrusakov 🎉 Your pull request was merged! Please take a moment to answer a two question survey so we can improve your experience in the future. |
# [20.0.0](v19.25.3...v20.0.0) (2022-06-17) * feat!: implement i18n in Paragon components and in docs site (#1100) ([53e0ac6](53e0ac6)), closes [#1100](#1100) ### BREAKING CHANGES * By adding i18n support to the Paragon design system, we are introducing a peer dependency on `[email protected]` or greater. This may be a breaking change for some consumers, if your repository: * Uses v1 of `@edx/frontend-platform` * Uses older version of `react-intl` than v5.25.0 directly. * Does not use `react-intl`.
🎉 This PR is included in version 20.0.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
This is an ongoing work to implement i18n support in Paragon.
To see i18n in action go to https://deploy-preview-1100--paragon-openedx.netlify.app/components/alert/?feature=LANGUAGE_SWITCHER, switch language in settings menu and see how the label of 'Dismiss' button of the
Alert
changes (there are sample translation for this label in this PR).JIRA: