Skip to content

Conversation

@M-DinhHoangViet
Copy link
Contributor

Fixes #13888

@fitztrev fitztrev self-assigned this Nov 6, 2023
Copy link
Member

@fitztrev fitztrev left a comment

Choose a reason for hiding this comment

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

Can you include a screenshot of the pages on your development site after the translations have been applied? That would show us that you successfully tested it and that the work is correct.

Comment on lines 152 to 166
<string name="goToLichess">Go to lichess.org</string>
<string name="browsersMozillaFirefox">Press Ctrl-i on linux/windows or cmd-i on macos</string>
<string name="browsersMozillaFirefox1">Click the Permissions tab</string>
<string name="browsersMozillaFirefox2">Allow Audio and Video on lichess.org</string>
<string name="browsersGoogleChrome">Click the lock icon in the address bar</string>
<string name="browsersGoogleChrome1">Click Site Settings</string>
<string name="browsersGoogleChrome2">Allow Sound</string>
<string name="browsersSafari">Click Safari in the menu bar</string>
<string name="browsersSafari1">Click Settings for This Website</string>
<string name="browsersSafari2">Allow Auto-Play</string>
<string name="browsersMicrosoftEdge">Click the three dots in the top right corner</string>
<string name="browsersMicrosoftEdge1">Click Settings</string>
<string name="browsersMicrosoftEdge2">Click Cookies and Site Permissions</string>
<string name="browsersMicrosoftEdge3">Scroll down and click Media autoplay</string>
<string name="browsersMicrosoftEdge4">Add lichess.org to Allow</string>
Copy link
Member

Choose a reason for hiding this comment

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

ornicar mentioned in the other PR that these list items can be consolidated.

Suggested change
<string name="goToLichess">Go to lichess.org</string>
<string name="browsersMozillaFirefox">Press Ctrl-i on linux/windows or cmd-i on macos</string>
<string name="browsersMozillaFirefox1">Click the Permissions tab</string>
<string name="browsersMozillaFirefox2">Allow Audio and Video on lichess.org</string>
<string name="browsersGoogleChrome">Click the lock icon in the address bar</string>
<string name="browsersGoogleChrome1">Click Site Settings</string>
<string name="browsersGoogleChrome2">Allow Sound</string>
<string name="browsersSafari">Click Safari in the menu bar</string>
<string name="browsersSafari1">Click Settings for This Website</string>
<string name="browsersSafari2">Allow Auto-Play</string>
<string name="browsersMicrosoftEdge">Click the three dots in the top right corner</string>
<string name="browsersMicrosoftEdge1">Click Settings</string>
<string name="browsersMicrosoftEdge2">Click Cookies and Site Permissions</string>
<string name="browsersMicrosoftEdge3">Scroll down and click Media autoplay</string>
<string name="browsersMicrosoftEdge4">Add lichess.org to Allow</string>
<string name="enableAutoplayForSoundsFirefox">1. Go to lichess.org
2. Press ctrl-i on linux/windows or cmd-i on macos
3. Click the Permissions tab
4. Allow Audio and Video on lichess.org</string>
<string name="enableAutoplayForSoundsChrome">1. Go to lichess.org
2. Click the lock icon in the address bar
3. Click Site Settings
4. Allow Sound</string>
<string name="enableAutoplayForSoundsSafari">1. Go to lichess.org
2. Click Safari in the menu bar
3. Click Settings for This Website
4. Allow Auto-Play</string>
<string name="enableAutoplayForSoundsMicrosoftEdge">1. Click the three dots in the top right corner
2. Click Settings
3. Click Cookies and Site Permissions
4. Scroll down and click Media autoplay
5. Add lichess.org to Allow</string>

Copy link
Contributor Author

@M-DinhHoangViet M-DinhHoangViet Nov 7, 2023

Choose a reason for hiding this comment

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

image
image

Copy link
Member

Choose a reason for hiding this comment

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

@M-DinhHoangViet Can you apply these changes? And then use these keys in faq.scala like this:

h3("Mozilla Firefox (", desktop(), "):"),
p(enableAutoplayForSoundsFirefox()),
h3("Google Chrome (", desktop(), "):"),
p(enableAutoplayForSoundsChrome()),
h3("Safari (", desktop(), "):"),
p(enableAutoplayForSoundsSafari()),
h3("Microsoft Edge (", desktop(), "):"),
p(enableAutoplayForSoundsMicrosoftEdge()),

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I did that and the same thing

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Apply the changes from my 2 comments above (1 and 2).

Put all the <li> content in a single block of text, separated by a newline, so they don't have to be translated as individual strings.

I did as you did and when I tried it in Gidpod and it showed up as I said

Copy link
Member

Choose a reason for hiding this comment

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

Ok, glad it worked. Can you commit those changes to this branch? They don't show in the pull request.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh no @fitztrev
image
image
image

Copy link
Member

Choose a reason for hiding this comment

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

@M-DinhHoangViet That's what we want. You can commit that. Then once we apply @schlawg's suggestion about adding nl2br, it will appear formatted correctly.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm testing now!

@M-DinhHoangViet
Copy link
Contributor Author

Can you include a screenshot of the pages on your development site after the translations have been applied? That would show us that you successfully tested it and that the work is correct.
Before:
image

After:
image

@schlawg
Copy link
Contributor

schlawg commented Nov 7, 2023

I'm not sure we want to combine the first three paragraphs just to save a few translation keys. At least not without edits to make the sentence flow less abrupt. As a native English speaker, I could make those edits.

But I'd much prefer to use the original text in three separate paragraphs.

@M-DinhHoangViet
Copy link
Contributor Author

I'm not sure we want to combine the first three paragraphs just to save a few translation keys. At least not without edits to make the sentence flow less abrupt. As a native English speaker, I could make those edits.

But I'd much prefer to use the original text in three separate paragraphs.

If so, if you want to combine them in one go, they cannot go down the line. See for example: https://lichess.org/faq#timeout, https://github.com/lichess-org/lila/blob/master/app/views/site/faq.scala#L155, https://github.com/lichess-org/lila/blob/master/translation/source/faq.xml#L51

@schlawg
Copy link
Contributor

schlawg commented Nov 8, 2023

Question for M-DinhHoangViet - is your given name Hoang or Viet? What's the best way to address you that's shorter than 15 characters?

@M-DinhHoangViet
Copy link
Contributor Author

M-DinhHoangViet commented Nov 8, 2023

Question for M-DinhHoangViet - is your given name Hoang or Viet? What's the best way to address you that's shorter than 15 characters?

Many people mistake my name as Dinh, and some mistake my name as Hoang. But actually my name is Viet

@schlawg
Copy link
Contributor

schlawg commented Nov 8, 2023

Hello Viet, I was referring to the first three paragraphs:

Paragraph 1:
            Most browsers can prevent sound from playing on a freshly loaded page to protect users. 
            Imagine if every website could immediately bombard you with audio ads.

Paragraph 2:
            The red mute icon appears when your browser prevents lichess.org from playing a sound.
            Usually this restriction is lifted once you click something. On some mobile browsers,
            touch dragging a piece does not count as a click. In that case you must tap the board to
            allow sound at the start of each game.

Paragraph 3:
            We show the red icon to alert you when this happens. Often you can explicitly allow
            lichess.org to play sounds. Here are instructions for doing so on recent versions of
            some popular browsers.

I understand that there is no obvious way to place paragraph breaks within a single translation. But for this text, when the paragraphs are combined at the view layer, the resulting text becomes harder to read and understand. So I'm wondering if we should treat each paragraph as its own translation so they can be rendered individually.

  p(mostBrowsersCanPreventSound()),
  p(theRedMuteIcon()),
  p(hereAreInstructions())

@M-DinhHoangViet
Copy link
Contributor Author

I want the same as you, but what about the opinions of @ornicar and @fitztrev

@schlawg
Copy link
Contributor

schlawg commented Nov 8, 2023

Apparently there is a function that can help us here.
Add import lila.common.String.html.nl2br near the other imports at the top of app/views/site/faq.scala
then you can do:

  p(nl2br(allThreeParagraphs())) // note to Viet - choose a better i18n key name :)

nl2br will parse the translation text and replace newlines with <br> which will separate the paragraphs.

@M-DinhHoangViet
Copy link
Contributor Author

Apparently there is a function that can help us here. Add import lila.common.String.html.nl2br near the other imports at the top of app/views/site/faq.scala then you can do:

  p(nl2br(allThreeParagraphs())) // note to Viet - choose a better i18n key name :)

nl2br will parse the translation text and replace newlines with <br> which will separate the paragraphs.

image
image
it not working

@fitztrev
Copy link
Member

fitztrev commented Nov 8, 2023

Try this line instead:

p(nl2br(enableAutoplayForSoundsA.txt())),

@fitztrev
Copy link
Member

fitztrev commented Nov 8, 2023

@M-DinhHoangViet To fix the build now you can run these 2 commands in gitpod in the terminal.

cd /workspace/lila-docker
./lila-docker format

That will fix the tab and space combination issue. Then you can commit the change.

@M-DinhHoangViet M-DinhHoangViet marked this pull request as draft November 9, 2023 05:09
@M-DinhHoangViet
Copy link
Contributor Author

done!

@M-DinhHoangViet M-DinhHoangViet marked this pull request as ready for review November 9, 2023 05:29
* master:
  rename CC button - closes lichess-org#13945
  don't expand images in streamer mod notes
  fix new lines in translated HTML
  New Crowdin updates (lichess-org#13939)
  remove non-working disable button from voice gear menu
  remove unnecessary junk
  same arch variable names as lila-ws/pekko
  arm64 native reactivemongo
`key()` instead of `nl2br(key.txt())`
@ornicar
Copy link
Collaborator

ornicar commented Nov 9, 2023

Well done everyone, that's a wrap

@ornicar ornicar merged commit 2b9c308 into lichess-org:master Nov 9, 2023
@M-DinhHoangViet M-DinhHoangViet deleted the patch-11 branch November 9, 2023 11:51
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.

4 participants