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

[UI] Make keyword toggle also switcher language of the editor #4828

Merged
merged 23 commits into from
Dec 1, 2023

Conversation

Felienne
Copy link
Member

@Felienne Felienne commented Nov 30, 2023

Fixes #4420

Some background

I want to describe a bit of background here because this is such a lovely example of a set of very reasonable decisions leading to bad design!

When we introduced the keyword languages, we added a switcher on the top of the screen, to switch all code examples in the adventures. So far so good. But we also switched start codes in the editor and translated them (since they have {}, the translation does not mean parsing but means grabbing from the yaml file and replacing the keywords). This happens when you have not written code yet. If you do, it does not load the startcode and then it does not translate.

But... Since we seems to translate code in the editor... people thought it could also translate their own code. It actually took me a long time to figure out why people (like @Mark-Giesen) thought that that is what the button would do!

This PR makes the one button translate in the editor too.

Limitations

The error handling in the translation code is currently quite limited. I tried to expand it in e08375d but it will involve extensive refactoring to do it nicely, so I prefer to do that separately.

I was also planning to Rename the top switcher so that it does not show the translation emoji, but the current keyword language (so: en or nl), but this leads to some design issues (#4828 (comment)) that I also want to address separately.

Changes

  • Makes the top switcher translate the program too
  • Removes the switcher in the editor for the admin
  • Gives an error message in the regular place instead of a shortlived modal
  • Show the top switcher, also when people are logged in, so they can use the button to switch it -> was already the case!

How to test
Set Hedy to any non-En language and write code in that language (f.e. slaap 20 in Dutch, level 4). Use the toggle and observe the keywords being translated.
Create a broken program with a parse error and see the error message appear below.

@ghost
Copy link

ghost commented Nov 30, 2023

👇 Click on the image for a new way to code review

Review these changes using an interactive CodeSee Map

Legend

CodeSee Map legend

@Felienne
Copy link
Member Author

We are getting there!

One issue though is that the keyword language is the code of the language in latin letters. Does not look pretty for, for example, Arabic:

image

@Felienne Felienne requested a review from jpelay November 30, 2023 06:00
@Felienne
Copy link
Member Author

Hi @jpelay!

I think this all works as expected (basically I just merged 2 functions) but I would like a sanity check on my typescript :)

Copy link
Member

@jpelay jpelay left a comment

Choose a reason for hiding this comment

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

Hi @jpelay!

I think this all works as expected (basically I just merged 2 functions) but I would like a sanity check on my typescript :)

So cool to have you working on the front-end too 😄 The typescript code looks fine! I have a comment not related to the front-end but to the translation process:

image

If the program has a syntax error, it doesn't get translated, and no message is shown. Perhaps we can translated the commands that are properly written or show a message? What do you think?

@Felienne
Copy link
Member Author

Felienne commented Dec 1, 2023

So cool to have you working on the front-end too 😄 The typescript code looks fine!

Thanks! I actually really likes writing a bit of typescript, I maybe want to do more of that going forward!

I have a comment not related to the front-end but to the translation process.
If the program has a syntax error, it doesn't get translated, and no message is shown. Perhaps we can translated the commands that are properly written or show a message? What do you think?

That is a great catch! I saw the front-end message appear so I assumed it would work always but it turned out it only worked for ParseErrors but not for error productions that are caught on the way! Turned out... (plot twist!) there was an old comment from me documenting this issue:

image (

# FH Feb 2022 TODO trees containing invalid nodes are happily translated,
)

Fixed that now!

@Felienne
Copy link
Member Author

Felienne commented Dec 1, 2023

A few issues remain (note to self):

  • Showing of None in the switcher button
  • Latin view of in the switcher button
  • Position of error box

@Felienne
Copy link
Member Author

Felienne commented Dec 1, 2023

So the error now works, but it shown on top:

image

And not in the regular error message place. I don't like that, I don't think it makes sense. Will try to see if my typescript skills reach wide enough to fix that :D

@Felienne Felienne marked this pull request as ready for review December 1, 2023 06:37
@Felienne Felienne requested a review from jpelay December 1, 2023 06:37
Copy link
Member

@jpelay jpelay left a comment

Choose a reason for hiding this comment

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

Everything works great now!!!

Copy link
Contributor

mergify bot commented Dec 1, 2023

Thank you for contributing! Your pull request is now going on the merge train (choo choo! Do not click update from main anymore, and be sure to allow changes to be pushed to your fork).

@mergify mergify bot merged commit b8a1294 into main Dec 1, 2023
11 checks passed
@mergify mergify bot deleted the language-switcher branch December 1, 2023 21:24
mergify bot pushed a commit that referenced this pull request Dec 12, 2023
(This is a redo of #4867 which contained some commits now already on main)

Since #4828 translation features are more prominently visible, so I figured we need to do a bit better. This actually changes some conceptual assumptions, so let's document here:

* Programs that have a ParseError or contain Placeholders, or have indentation issues, still cannot be translated (since they can't be parsed and then we don't have a tree to work wtih)
* All other errors however create a parsetree with error nodes, and thus... can just be translated keeping the error nodes, so that is what we now do!

**How to test**

Try a program with an error production, f.e. in German, level 7:
`repeat 3 times 'Hedy macht Spaß!'`

Main shows an error:
<img width="724" alt="image" src="https://github.com/hedyorg/hedy/assets/1003685/2ec4d071-9c1e-4e10-9a9b-606024e42e37">

This PR translates the code:
<img width="693" alt="image" src="https://github.com/hedyorg/hedy/assets/1003685/4d3c4410-900c-4b98-99f2-502672e42420">
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.

[FEATURE] Show translation switcher for some users
2 participants