Skip to content

add multiple language selection #1148

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

Merged
merged 8 commits into from
Apr 29, 2017
Merged

add multiple language selection #1148

merged 8 commits into from
Apr 29, 2017

Conversation

lcxfs1991
Copy link
Collaborator

@lcxfs1991 lcxfs1991 commented Apr 23, 2017

Preview:

language-webpack

Fixes #966

@lcxfs1991
Copy link
Collaborator Author

@bebraw Please take a look

@simon04 simon04 added the UI/UX label Apr 23, 2017
@simon04 simon04 requested review from skipjack and bebraw April 23, 2017 12:33
Copy link
Collaborator

@skipjack skipjack left a comment

Choose a reason for hiding this comment

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

Ok so two things right off the bat:

  • The build is failing because you can't import a stylesheet within antwar's <Interactive> component. I know you're not using <Interactive> directly but the Navigation component is. You'll have move the import './drop-down-style'; into the Site component (where you'll see a bunch of other style imports.
  • Also, did you test this on the mid-range breakpoint (from 768 to 1024)? The main navigation section is already a bit crowded and I have a feeling adding another large link will either break the layout or make things look even more tight. I've been giving this some thought since you opened the issue and I think a tighter menu like this or this (maybe using abbreviations instead of flags) would be the right approach.

Once the import is resolved and we discuss the layout I'd be happy to do a fuller review/test. If need be, I can definitely help with this to get it across the finish line. Aside from the points I mentioned there's a few other design tweaks I might make but that can be done in a separate PR.

@skipjack
Copy link
Collaborator

@bebraw @sokra @TheLarkInn @lcxfs1991 also, from a higher level, how does internationalization play out in terms of issue management, prs, etc?

I know right now there's a translation branch in this repo and there's been some talk of using locki, #295, but I'm not sure where we're at with that. It seems there's a separate repo for the chinese fork (which I think makes sense for management purposes) but then what's the local translation branch for?

@lcxfs1991
Copy link
Collaborator Author

lcxfs1991 commented Apr 27, 2017

Local translation branch is for a separate management just like webpack-china since we have to fetch plugin/loader readme and put them under /content/ folder. However, the master branch generate directly. It is a bit hard for us to make a diff if we simply follow what master do.

I think we can try locki. But we need to ensure the performance and we'd better keep a local copy of data.

We can then compare two approaches, locki or webpack-china, and decide which one we should pick up for localization.

@skipjack
Copy link
Collaborator

skipjack commented Apr 27, 2017

@lcxfs1991 ah ok thanks for the clarification, I did see something about that in #722 I think. In terms of the getting this PR merged please see my comments above.

@bebraw any reason we can't just keep that change in the master branch? I actually kind of like having all site content, including fetched content, in the /content directory. Is this to keep a visual distinction for contributors so they know not to edit it?

@lcxfs1991
Copy link
Collaborator Author

Let me fix this tonight.

@skipjack
Copy link
Collaborator

Ok great, no rush.

@lcxfs1991
Copy link
Collaborator Author

lcxfs1991 commented Apr 28, 2017

width: 768
768
width: 1024
1024
hover:
image

@skipjack pls check.

@skipjack
Copy link
Collaborator

@lcxfs1991 awesome work, I will review again (and hopefully merge) shortly!

@skipjack skipjack merged commit 371a6be into webpack:master Apr 29, 2017
This was referenced Apr 29, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants