Skip to content
This repository has been archived by the owner on Mar 1, 2019. It is now read-only.

languages: interface, server implementation #105

Merged
merged 6 commits into from Dec 14, 2018
Merged

languages: interface, server implementation #105

merged 6 commits into from Dec 14, 2018

Conversation

wiese
Copy link
Contributor

@wiese wiese commented Dec 7, 2018

Provide languages and their directionality to the application.

Use this as base to set languages in the client.

Bug: T210407

@wiese wiese requested review from jakobw and bitPogo December 7, 2018 12:22
Copy link
Contributor

@bitPogo bitPogo left a comment

Choose a reason for hiding this comment

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

Seems good for me

@wiese
Copy link
Contributor Author

wiese commented Dec 11, 2018

Rebased

Copy link
Member

@jakobw jakobw left a comment

Choose a reason for hiding this comment

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

code lgtm, although once something is shown I would find it a little awkward to have a hardcoded list of languages.

(needs a rebuild)

@wiese wiese changed the title store: implement LANGUAGE_UPDATE mutation [WIP] store: implement LANGUAGE_UPDATE mutation Dec 11, 2018
@wiese wiese changed the title [WIP] store: implement LANGUAGE_UPDATE mutation [WIP] languages: interface, server implementation Dec 12, 2018
@wiese wiese changed the base branch from master to help-travis December 13, 2018 10:52
new EntityInitializer(),
),
);
const languageRepo = new WikibaseContentLanguagesRepo(
Copy link
Contributor Author

@wiese wiese Dec 13, 2018

Choose a reason for hiding this comment

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

Had we continued doing this outside the export, all requests would have a shared the same language API response. This could be, carefully crafted, a performance optimization in the future but should be an active decision. (Not currently possible for this scenario with the API response containing request-specific content [translation] but the deferred member always resolving to the same value once it is known)

@wiese wiese changed the title [WIP] languages: interface, server implementation languages: interface, server implementation Dec 13, 2018
@wiese wiese changed the base branch from help-travis to master December 13, 2018 12:15
@wiese
Copy link
Contributor Author

wiese commented Dec 13, 2018

Rebased

Copy link
Member

@jakobw jakobw left a comment

Choose a reason for hiding this comment

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

lgtm in general!

@@ -14,7 +14,8 @@
"quotemark": [true, "single"],
"interface-name": false,
"ordered-imports": false,
"object-literal-sort-keys": false
"object-literal-sort-keys": false,
"import-spacing": false
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The long class names, combined with the max-line limit made this necessary.
Unfortunately import-spacing is not configurable any more than on/off, so it is unclear how iffy it allows the header to looks, but I still perceived it as the lesser evil to allowing very long lines everywhere or plastering all uses with config comments.
If someone has a better idea I'd be happy to change that.

@wiese
Copy link
Contributor Author

wiese commented Dec 14, 2018

Updated to address @jakobw's valid concerns.

Copy link
Member

@jakobw jakobw left a comment

Choose a reason for hiding this comment

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

Gave it another look. I like that the evilness is now confined in WaitingForLanguageWikibaseContentLanguagesRepo.

Add the LANGUAGE_UPDATE mutation and use it with dummy data from the
LANGUAGE_INIT action

Bug: T210407
Implements the LanguageRepository interface for the server side,
sharing an API response with the implementation of
LanguageTranslationRepository which happens to contain all the necessary
information (and some additional in user language).

Bug: T209288
Use https://www.npmjs.com/package/rtl-detect to determine language
script directionality.

Bug: T209288
@wiese
Copy link
Contributor Author

wiese commented Dec 14, 2018

Rebased

@jakobw jakobw merged commit f120afa into master Dec 14, 2018
@jakobw jakobw deleted the lang-update branch December 14, 2018 18:47
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants