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

Very rough approach for implementing language settings #3742

Merged
merged 6 commits into from
Oct 29, 2019

Conversation

rowanwins
Copy link
Contributor

@rowanwins rowanwins commented Oct 23, 2019

Related to #3707 where it's suggested to changing the naming of some buttons are parts of the interface.

This approach would see languageOverrides to be passed in from TerriaMap to via the options to Terria.js

The main downside I've seen so far is that some text components occasionally include React components like Icons, for example empty workbench has a message which includes a lightbulb icon. The current approach doesn't render those react components. Someone much better with React would probably have a few ideas how to handle this.

What I've done in TerriaMap/index.js is

import languageOverrides from './lib/Language/overrides';

terriaOptions.languageOverrides = languageOverrides;

These overrides then get merged into the defaults.

Anyway this is my very naive approach :)

CustomLanguage

@rowanwins rowanwins changed the title Very rough approach for implementing language settings [WIP]: Very rough approach for implementing language settings Oct 23, 2019
@soyarsauce
Copy link
Contributor

So there's a few ways around the icon problem, e.g. feeding in an actual react component as part of the "i18n"/language config, or the simplest way for now is to keep the existing components as "defaults" (with icon), then overrides means no icon-ability atm.

@rowanwins
Copy link
Contributor Author

I'll do a quick audit tomorrow of how many places we use components within regular HTML, if it's only the lightbulb icon in that empty workbench effected then I'm happy to have no icon-ability :)

@kring
Copy link
Member

kring commented Oct 23, 2019

@rowanwins @soyarsauce why not use https://github.com/TerriaJS/terriajs/blob/master/lib/ReactViews/Custom/parseCustomMarkdownToReact.js to render the localized "strings". Then you can support icons by defining a custom component (https://github.com/TerriaJS/terriajs/blob/master/lib/ReactViews/Custom/registerCustomComponentTypes.js). It sanitizes the input too.

@rowanwins
Copy link
Contributor Author

Good idea @kring - I'll give it a whirlybird!

@rowanwins
Copy link
Contributor Author

rowanwins commented Oct 24, 2019

Okay @kring & @soyarsauce - the latest commit adds support for Icon's as customComponents.

The other component that comes to mind that might be nice to support via custom text is the Button component, eg I'm thinking about the possibility of buttons in that empty workbench area...

Before I add support for the Buttons do you want to make sure you're happy with that handling for the Icon component?
EDIT: We don't use a fancy button component, just regular old buttons with a few classes work fine!

And are we happy to proceed and get this into master?

On a sidenote there is also some similar stuff that we do via the config.json in TerriaMap (eg we have a feedbackPreamble option - who knew!! At some stage it would be good to rationalise.

@rowanwins rowanwins changed the title [WIP]: Very rough approach for implementing language settings Very rough approach for implementing language settings Oct 28, 2019
@rowanwins
Copy link
Contributor Author

Theoretically the Icon.jsx also takes a style object as a prop however this would be a smidge fiddily to support as we'd basically be taking a string and converting it to a object. I reckon we just support styling via the className prop.

@AnaBelgun
Copy link
Member

reminder to @soyarsauce to review this one

@soyarsauce
Copy link
Contributor

This is awesome work @rowanwins & looks good, hope you don't mind I just extended so we can also use any component/jsx, if the latest commit looks good to you, let's merge it
a7305b0

@soyarsauce soyarsauce assigned rowanwins and unassigned soyarsauce Oct 28, 2019
@rowanwins
Copy link
Contributor Author

Looks good to me @soyarsauce - it doesn't seem like I can merge it because I can't review it because I opened the PR, but I've looked over it and tested locally and it all makes sense.
I'll open a corresponding PR over in the TerriaMap repo.

@soyarsauce
Copy link
Contributor

Sorry @rowanwins I thought I approved w/ my single comment review

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants