Skip to content

Conversation

@oruburos
Copy link
Collaborator

@oruburos oruburos commented Aug 7, 2020

I have verified that this pull request:

  • has no linting errors (npm run lint)
  • is from a uniquely-named feature branch and has been rebased on top of the latest master. (If I was asked to make more changes, I have made sure to rebase onto master then too)

This pull request:

  • introduces translations in Aria labels in toolbar component.
  • It also introduces the i18n test file to deal with the toolbar.test

oruburos and others added 21 commits July 22, 2020 03:03
Translation with new namespace
I18Next configuration leaning on default separator and namespace

Broom: i18n + debug:false
Test entry for Toolbar.test.jsx
About : broom About lines 17-26
Nav component : changes in keys
KeyboardShortcutModal.jsx: Key now in Common
Labels included in translations.json
Labels included in translations.json
Labels included in translations.json
Labels included in translations.json
Snapshot updated npm run test -- -u
Updated names to call the labels
Common namespace without currently used entries
Missing Common.p5logoARIA key
Deleting commented line 78
…nav_2

# Conflicts:
#	client/modules/IDE/components/Toolbar.jsx
@oruburos oruburos changed the title Aria Labels in Toolbar ARIA labels in Toolbar Aug 7, 2020


import { ToolbarComponent } from './Toolbar';
import { i18ntest } from '../../../i18n-test';
Copy link
Member

Choose a reason for hiding this comment

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

If you're not using the i18ntest variable, then there's no need to assign it here. You can just do this:

import '../../../i18n-test';

this.props.setGridOutput(true);
}}
aria-label="Play sketch"
aria-label={i18n.t('Toolbar.PlaySketchARIA')}
Copy link
Member

Choose a reason for hiding this comment

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

I think we should use withTranslation() here. The react-i18next documentation doesn't mention the method you're using here at all which makes me concerned about using it.

All of these can be this.props.t('...')

@oruburos
Copy link
Collaborator Author

@andrewn I made the changes requested, but the tests are failing in toolbar.test, do you have an idea what could fix this? Can you take a look please?

@andrewn andrewn mentioned this pull request Aug 22, 2020
3 tasks
@andrewn
Copy link
Member

andrewn commented Aug 22, 2020

@oruburos I'm going to close this as I extracted the relevant commits from this PR and made the tests work. Can you review #1568 please?

@andrewn andrewn closed this Aug 22, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants