Skip to content

Add language selection.#1

Closed
mmai wants to merge 1 commit inton1k0:mainfrom
mmai:main
Closed

Add language selection.#1
mmai wants to merge 1 commit inton1k0:mainfrom
mmai:main

Conversation

@mmai
Copy link
Copy Markdown

@mmai mmai commented Jan 8, 2022

This add the possibility to choose alternates dictionaries.

See the result here : https://mmai.github.io/wordlem/
There is probably a way to make that prettier.

Copy link
Copy Markdown
Owner

@n1k0 n1k0 left a comment

Choose a reason for hiding this comment

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

Hey, this is awesome! Could you please install elm-format and apply automatic code formatting so we can land this? I'll probably take over next to tweat the styles a little, but thank you so much for this great patch 🎉

Comment thread src/Main.elm
main =
Browser.element
{ init = init
{ init = init English
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

We could probably detect users language from their browser environment, eg by using navigator.language and passing it to Flags, so we would preselect their language by default!

Comment thread src/Main.elm
Comment on lines +361 to +362
toString : Language -> String
toString language =
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

maybe languageToString so it's a little more explicit/less vague?

Comment thread src/Main.elm
( { model | currentTry = string }, Cmd.none )

SwitchLanguage lang ->
update NewGame { model | language = lang }
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

It's usually better to avoid calling update recursively, so here we could just return ( { model | language = lang }, Cmd.none ) :)

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Ah no my bad, we need to init indeed. Scratch it, we won't bother destructuring just to update language here 👍

Comment thread src/Main.elm
Comment on lines +254 to +259
let classes = if isActive then
"btn btn-secondary active"
else
"btn btn-secondary"
in
label [ class classes ]
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

You have a convenient classList helper which may be easier to use here https://package.elm-lang.org/packages/elm/html/latest/Html-Attributes#classList

Comment thread src/Main.elm
Comment on lines +99 to +100
try : Language -> WordToFind -> UserInput -> Result String Attempt
try lang word input =
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

There's probably an error string to update as well in this function:

        Err <| "Sorry, " ++ input ++ " must be a word known from our dictionary"
-- -----------------------------------------------------------------^ French/English

Also btw in English language names always take an uppercase letter first.

@n1k0
Copy link
Copy Markdown
Owner

n1k0 commented Jan 9, 2022

I took a lot of inspiration from your patch and went with a laaaarge refactor of the previous codebase. Unfortunately I couldn't cherry pick your commits as the diff was way too important, but thanks again for the ideas! FWIW I've credited you in the README, hope it's okay for you. Cheers!

@n1k0 n1k0 closed this Jan 9, 2022
@mmai
Copy link
Copy Markdown
Author

mmai commented Jan 9, 2022

Yes, thanks!

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