Skip to content
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

fixing language in vocabulary select (multilingual mode) #1824

Merged

Conversation

dzonidoo
Copy link
Contributor

SDESK-6999

@tomaskikutis tomaskikutis changed the title work in progress fixing language in vocabulary select (multilingual mode) Jul 12, 2023
@@ -32,7 +33,10 @@ export default class CustomVocabulariesFields extends React.PureComponent<IProps
errors,
diff,
} = fieldProps;
const language = get(diff, 'language') || getUserInterfaceLanguageFromCV();

const language = MAIN_LANGUAGE ?? get(diff, 'language') ?? getUserInterfaceLanguageFromCV();
Copy link
Member

Choose a reason for hiding this comment

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

Hi @MarkLark86 I want to consult regarding the approach to fix this ticket. In multilingual mode there is the component to select the main language. When the language is changed, it saves it to the state. BUT when we are rendering a vocabulary select, we use item.language. Should we fix it by always updating item.language when the value of language select component changes? Or should we rather not touch item.language and use that state variable that stores the main language?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Hi @tomaskikutis & @dzonidoo

This Main Language selector is purely for view purposes and nothing to do with the data layer, therefor my recommendation would be to use the Editor's state.

This component is rendered by the Editor, using the EditorFieldCustomVocabularies component. The Editor passes in the language that should be used for rendering (see IEditorFieldProps for the interface).

My suggested solution here would be to:

  • Remove any language logic from within this component
  • Add a language prop to this component
  • Pass through the language prop in the EditorFieldCustomVocabularies component

This way the language used in this component is controlled elsewhere, in this case the Editor itself.

const language = MAIN_LANGUAGE ?? get(diff, 'language') ?? getUserInterfaceLanguageFromCV();

console.log(diff, 'diff');
const language = this.props.language ?? getUserInterfaceLanguageFromCV();
Copy link
Member

Choose a reason for hiding this comment

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

move getUserInterfaceLanguageFromCV() out of this file - apply it before passing language prop to this file

opt[searchKey].toLowerCase().indexOf(valueNoCase) >= 0
));

let searchResults = this.props.options.filter((opt) => {
Copy link
Member

Choose a reason for hiding this comment

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

try removing code duplication here: use a conditional for setting a function that would get a label in a correct language, but reuse the same label comparison logic without duplicating code

@tomaskikutis
Copy link
Member

@dzonidoo remember to remove draft state of a PR when it's ready for review image

I've pushed one commit to clean up your function. In your version, filtering algorithm was conditional(via if/else) when in fact it the same one was being used in both cases, the only difference being the label. I've made the code match the intention - label is conditional and filtering algorithm is the same.

Also in your version, the function name was called itemTranslation when the function itself wasn't doing anything specific to translations.

@tomaskikutis tomaskikutis marked this pull request as ready for review July 19, 2023 11:05
@dzonidoo dzonidoo merged commit 3f90070 into superdesk:develop Jul 20, 2023
14 checks passed
@MarkLark86 MarkLark86 added this to the 2.7 milestone Feb 19, 2024
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.

3 participants