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)
  • is descriptively named

 translations.json (both languages EN - ES)
 changes in NewFileForm.jsx and NewFileModal.jsx to link the new keys.
 translations.json (both languages EN - ES)
 changes in NewFileForm.jsx and NewFileModal.jsx to link the new keys.
@oruburos oruburos changed the title Modals for Sketch Menu, Modals for Sketch Menu Aug 7, 2020
@oruburos oruburos changed the title Modals for Sketch Menu Spanish Translation Modals for Sketch Menu Aug 7, 2020
id="name"
type="text"
placeholder="Name"
placeholder={i18n.t('NewFileForm.Placeholder')}
Copy link
Member

Choose a reason for hiding this comment

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

Same comments as in #1537. In components, let's try and use withTranslation.

The validate() function is a good case for using the i18next.t() function directly but here it should be this.props.t().

Copy link
Collaborator Author

@oruburos oruburos Aug 11, 2020

Choose a reason for hiding this comment

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

@andrewn I made the changes requested.

Copy link
Member

@andrewn andrewn left a comment

Choose a reason for hiding this comment

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

See comments in code

@oruburos
Copy link
Collaborator Author

@andrewn I made the changes requested. The problem was also present in the Folder modal. Can you take a look, please?

@andrewn
Copy link
Member

andrewn commented Aug 11, 2020

Looks great!

@andrewn andrewn merged commit 3253770 into processing:develop Aug 11, 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