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

[Feature] copy button on intro #17

Merged
merged 10 commits into from Jun 28, 2020
Merged

Conversation

samslow
Copy link
Contributor

@samslow samslow commented Jun 27, 2020

image
image

resolve #16

Add copy button on create room button.

if you click this, system write roomId on your clipboard.


Hello. this is my first PR on this game.

I tried to follow the conventions of the project as much as possible.

But if I'm lacking anything, please add comment. cause i'm a just student.

  • Is there a special reason for distinguishing the two language file by index.html each folder?
    If I were you, I would have applied different languages to one file.

@ghk829
Copy link

ghk829 commented Jun 27, 2020

@samslow
wow, this PR is exactly the one I wish to make.

@gorisanson
Copy link
Owner

gorisanson commented Jun 27, 2020

  • Is there a special reason for distinguishing the two language file by index.html each folder?
    If I were you, I would have applied different languages to one file.

Could you be more specific? What other method can I use to support multiple languages?

@samslow
Copy link
Contributor Author

samslow commented Jun 28, 2020

NOTE. That's just Question. not be related this PR.

um.. current directory structure is as follows

image

yes. that's almost same structure

If I update ko/index.html, I have to update en/index.html as well.

if we get each doc's language state, we can merge that two file i think.

I thought it might have been intended, so I was wondering if you could explain it if you did.

@gorisanson
Copy link
Owner

gorisanson commented Jun 28, 2020

NOTE. That's just Question. not be related this PR.

I know it. I'm just interested in the better way of managing a page supporting multiple languages. And I think reviewing your PR would also be done more smoothly after talking about it, since the way you dealt with multiple languages in your PR is different from the way I have been doing on this project.

if we get each doc's language state, we can merge that two file i think.

In what way can we merge? Can you give me some guide, please?

I thought it might have been intended, so I was wondering if you could explain it if you did.

I don't understand this sentence. What should I explain?

@gorisanson
Copy link
Owner

gorisanson commented Jun 28, 2020

if we get each doc's language state, we can merge that two file i think.

Maybe you mean the option 4, while my current way of doing is option 2, on this stack overflow answer?

@samslow
Copy link
Contributor Author

samslow commented Jun 28, 2020

Maybe you mean the option 4, while my current way of doing is option 2, on this stack overflow answer?

Oh. that's good example.

this project's way is Options 2 described as a nightmare.

and my suggestion is not exist that stackoverflow.

My way is to put a separate language pack file or enum and render it dynamically in a language state.

For this, it would be easier for us to use frameworks such as React or Vue.

But if we want more easy, we can make language file and one index.html

ps. i think merge this PR first. And open another Issue for the discussion about language.

@gorisanson
Copy link
Owner

My way is to put a separate language pack file or enum and render it dynamically in a language state.

Sorry for distraction, but, is there a link to a webpage or something which describes your method?

Copy link
Owner

@gorisanson gorisanson left a comment

Choose a reason for hiding this comment

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

Thank you for your PR! There're some changes I want. If there's something tiresome for you, I'll do it.

.gitignore Outdated Show resolved Hide resolved
src/resources/js/ui_online.js Outdated Show resolved Hide resolved
src/resources/js/ui_online.js Outdated Show resolved Hide resolved
@samslow
Copy link
Contributor Author

samslow commented Jun 28, 2020

@gorisanson

i can't find any example link.

So I just wrote it myself.

// app.js

import Language from '../config/language'

const app = () => {
  const currentLang = Language[getCurrentLang()] // "ko"
  render(
  	<html>
      ...
    	<body>
        <div>${currentLang.createRoom}</div>
        <div>${currentLang.roomId}</div>
        <div>${currentLang.copy}</div>
        ...
      </body>
    </html>
  )
}
// config/language.js

export default Language = {
  ko:{
    ...,
    "createRoom": "방 만들기",
    "roomId": "방 ID",
    "copy": "복사",
    ...
  },
  en:{
    ...,
    "createRoom": "create room",
    "roomId": "Room ID",
    "copy": "Copy",
    ...
  }
}

But This is not complete example, i want you this code explain my way to you

@gorisanson
Copy link
Owner

@samslow

Thanks for your detailed explanation! Now I got something!

I think if your code is rendered on the browser, then it is option 1 in the stack overflow answer. And if the html files are built from your code, then it is option 4 in the stack overflow answer. Is my understanding right?

@samslow
Copy link
Contributor Author

samslow commented Jun 28, 2020

@gorisanson

Thanks for your detailed explanation! Now I got something!
I think if your code is rendered on the browser, then it is option 1 in the stack overflow answer. And if the html files are built from your code, then it is option 4 in the stack overflow answer. Is my understanding right?

yes I think you can do that depending on anyone understanding.

if you don't want add any framewrok and only want vanila, we can apply that in <script/>

@samslow samslow requested a review from gorisanson June 28, 2020 08:46
Copy link
Owner

@gorisanson gorisanson left a comment

Choose a reason for hiding this comment

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

Now it looks good! Thank you again for this PR!

@gorisanson gorisanson merged commit 86e8f65 into gorisanson:master Jun 28, 2020
gorisanson added a commit that referenced this pull request Jun 28, 2020
@gorisanson
Copy link
Owner

@samslow

The revert commit is my mistake. It is not merged to branch master.
I just released new version which includes your copy button after changing the style of the button a little bit. Please check it!
Thank you for your PR again!

@samslow
Copy link
Contributor Author

samslow commented Jun 28, 2020

@samslow

The revert commit is my mistake. It is not merged to branch master.

I just released new version which includes your copy button after changing the style of the button a little bit. Please check it!

Thank you for your PR again!

i love it! good job!

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.

room seed number copy button
3 participants