Feature/8418 content language #671
Conversation
There was a problem hiding this comment.
Looking solid!
I have some suggestions based on patterns and anti-patterns.
The biggest: A component that commits a mutation must be a fragment container. Please make everything a fragment container.
Second is something dear to my heart: a mutation should do exactly one thing. Inline, I suggested two mutation functions for LanguageListItem.js. It might be even nicer to make make sub-components (SetDefaultLanguageAction, DeleteLanguageAction) with one mutation per action.
The final theme: I got confused by languagesList and Team.get_languages. I've put recommendations inline. Client-side, I think we should have a LanguageRegistry.js with helper functions. API-side, can we make Rails return Team.languages, an Array of Strings, and then obliterate Team.get_languages? @caiosba?
Yes, this is happening in a branch that is refactoring some things on the GraphQL layer. |
| disabled: PropTypes.bool, | ||
| checkBoxLabel: PropTypes.object, | ||
| continueButtonLabel: PropTypes.object, | ||
| checkBoxLabel: PropTypes.oneOfType([ |
There was a problem hiding this comment.
I feel this should be PropTypes.node (which captures both).
| PropTypes.string, | ||
| PropTypes.element, | ||
| ]), | ||
| continueButtonLabel: PropTypes.oneOfType([ |
| const options = Object.keys(LanguageRegistry) | ||
| .filter(code => !languages.includes(code)) | ||
| .sort((a, b) => compareLanguages(null, a, b)); | ||
| const getOptionLabel = code => `${languageLabel(code)} (${code})`; |
There was a problem hiding this comment.
"X (Y)" should be a <FormattedMessage>. Ref: https://www.mediawiki.org/wiki/Localisation#Symbols.2C_colons.2C_brackets.2C_etc._are_parts_of_messages
There was a problem hiding this comment.
Interesting! Looks like I'll need to use injectIntl and props.intl on this one as the Autocomplete component expects a string and performs .toLowerCase on it
There was a problem hiding this comment.
Good catch. I think you can call it renderOption instead. (The default renderOption() implementation calls getOptionLabel(). I don't see why there are two methods in the first place....)
| team { | ||
| id | ||
| get_language | ||
| get_languages |
There was a problem hiding this comment.
I don't think this changes ... does it?
| value="languages" | ||
| /> | ||
| : null } | ||
| {UserUtil.myRole(this.getCurrentUser(), team.slug) === 'owner' ? |
There was a problem hiding this comment.
Should be currentUserIsOwner
No description provided.