Closed Bug 900957 Opened 11 years ago Closed 11 years ago

Localize Popcorn Maker

Categories

(Webmaker Graveyard :: Popcorn Maker, defect)

Product:
Webmaker Graveyard
Component:
Popcorn Maker
Platform:
x86
macOS
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(Not tracked)

Status:
RESOLVED FIXED

People

(Reporter: alicoding, Assigned: alicoding)

References

Details

(Whiteboard: 20130805 p=1)

Attachments

(1 file, 1 obsolete file)

https://github.com/mozilla/popcorn.webmaker.org/pull/162
56 bytes, text/x-github-pull-request
humph
: review+
mjschranz
: review+
Details | Review 
This is a meta ticket on localizing Popcorn.webmaker.org
Status: NEW → ASSIGNED
One thing left on this is to localize the plugins and that will require some coordination with Matt and/or Humphrey because of the way how the plugin is written it might require some extra work in order to localize that.

Also, we might have to figure out about why `localized.js` failed sometime on the page load, but most of the part it is ready to be review.
Attachment #785387 - Flags: review?(schranz.m)
Attachment #785387 - Flags: review?(david.humphrey)
Comment on attachment 785387 [details] [review]
https://github.com/mozilla/popcorn.webmaker.org/pull/162

This patch is still plenty on going, but I'm going to R- it right now with a few notes.

1) I tried pulling this down locally and running with with thai but none of the strings were being translated.
2) The changes to URL structure when doing <HOSTNAME>/th-TH/editor/<SOME_ID>/edit is causing no project data to be retrieved back.
Attachment #785387 - Flags: review?(schranz.m) → review-
Also, lint errors:

public/src/butter.js: line 944, col 1, Trailing whitespace.
public/src/butter.js: line 990, col 15, Trailing whitespace.

public/src/core/localized.js: line 20, col 6, Trailing whitespace.

public/src/core/project.js: line 198, col 18, '_isRemix' is not defined.
public/src/core/project.js: line 274, col 9, '_isRemix' is not defined.

routes/index.js: line 8, col 4, Mixed spaces and tabs.
routes/index.js: line 9, col 6, Missing semicolon.
Whiteboard: 20130805 p=1
Linting problem fixed. 

Still not sure why you can't get it works on your local machine, but still working on #2.
Comment on attachment 785387 [details] [review]
https://github.com/mozilla/popcorn.webmaker.org/pull/162

Flagging you back to review this patch @matt.

We have fixed the project not retrieving the content correctly.
Attachment #785387 - Flags: review- → review?(schranz.m)
Comment on attachment 785387 [details] [review]
https://github.com/mozilla/popcorn.webmaker.org/pull/162

I've done a first pass, looking pretty good!

One thing I'd like to have you confirm is that you've audited all the <a href="..."> for the inclusion of {{lang}}.  I found a few spots where it looked like it was missing, and there might be more.  It would be good to make sure we haven't missed any.
Attachment #785387 - Flags: review?(david.humphrey) → review-
Attachment #785387 - Flags: review- → review?(david.humphrey)
Comment on attachment 785387 [details] [review]
https://github.com/mozilla/popcorn.webmaker.org/pull/162

Still some issues in this.

I'm also wanting an answer to the question I asked before, namely, are we sure we're not missing {{lang}} on href URLs anywhere?

Leaving Matt flagged, as I want him to review this for correctness, make sure it works right, etc.  He knows this code better than me.

Really close now, Ali...
Attachment #785387 - Flags: review?(david.humphrey) → review-
Comment on attachment 785387 [details] [review]
https://github.com/mozilla/popcorn.webmaker.org/pull/162

I've fixed everything as :humph asks and I have searched through all the files to see if any missing {{lang}} found and I believe all of them are good now.
Attachment #785387 - Flags: review- → review?(david.humphrey)
Comment on attachment 785387 [details] [review]
https://github.com/mozilla/popcorn.webmaker.org/pull/162

Awesome work, Ali.  Well done!

I'll pass the torch to Matt.  When he's happy, let's get this on staging.  Please don't push this to prod if you don't have to until next week (i.e., don't land late on a Friday :)
Attachment #785387 - Flags: review?(david.humphrey) → review+
Blocks: 903597
This got merged without Matt's r+?  What happened there?
I think matt was busy helping me check/rebase/push and forgot to r+ in here @humph
Commit pushed to master at https://github.com/mozilla/popcorn.webmaker.org

https://github.com/mozilla/popcorn.webmaker.org/commit/9226caaf9b154eac7be7e8dc98efcdf8aec7195e
Revert "Bug900957 - Localize Popcorn Maker"

This reverts commit d3d39ae5de58f32e4a84548a013b149783ca0fe7.
Looks like the reason this failed to work on staging was because something doesn't work with requirejs middleware:

HTTP Server started on http://localhost:8888
Press Ctrl+C to stop
compilation failed
[Error: ReferenceError: document is not defined
In module tree:
    butter
      modules
        editor/module
          editor/editor
            editor/trackevent-editor
              l10n
                core/localized

    at eval (eval at <anonymous> (/Users/jon/Sites/popcorn.webmaker.org/node_modules/requirejs-middleware/node_modules/requirejs/bin/r.js:22404:38), <anonymous>:6:21)
]
compilation failed
[Error: Error: ENOENT, no such file or directory '/Users/jon/Sites/popcorn.webmaker.org/public/src/layouts/controls.html'
In module tree:
    embed
      ui/widget/controls
        text

    at Object.fs.openSync (fs.js:427:18)
]
To see those errors, run `npm install` to update to v0.1.1 of requirejs-middleware which displays error messages, and set "OPTIMIZE_JS": true in your local.json file.
This is the stack trace for the latest compile build.

`once` set
compiling now
`once` set
compiling now
`once` set
compiling now
HTTP Server started on http://localhost:8888
Press Ctrl+C to stop
compilation failed for /var/folders/84/p794542s3bqctphmysv734y00000gn/T/mozilla.butter/src/butter.js:
Error: ReferenceError: document is not defined
In module tree:
    butter
      modules
        editor/module
          editor/editor
            editor/trackevent-editor
              l10n
                core/localized

    at eval (eval at <anonymous> (/Users/alihuta2002/work/popcorn.webmaker.org/node_modules/requirejs-middleware/node_modules/requirejs/bin/r.js:22404:38), <anonymous>:6:21)

compilation succeeded for /var/folders/84/p794542s3bqctphmysv734y00000gn/T/mozilla.butter/src/embed.js
compilation succeeded for /var/folders/84/p794542s3bqctphmysv734y00000gn/T/mozilla.butter/templates/assets/editors/editorhelper.js
I'm doing something wrong, as I can't reproduce this.  Can I get better STR?
@humph if you are actually fetch this branch and once you run the server this message will come up right after running the server on the terminal's console.
Not for me it doesn't.
Do you see these humph? 

`once` set
compiling now
`once` set
compiling now
`once` set
compiling now
I'm not sure why you don't see that, but you have to also reinstall node_modules if you haven't done that. There is an update on the package.json.
You need to set OPTIMIZE_JS to true in your local.json. Otherwise the requirejsmiddleware is not run.
So I was trying to see what causes the problem and found that this file is the one 

l10n.js

Weird that `document` is undefined when it's use in there or even `window`.
Comment on attachment 785387 [details] [review]
https://github.com/mozilla/popcorn.webmaker.org/pull/162

Last round of review please :D
Attachment #785387 - Flags: review+ → review?(david.humphrey)
Comment on attachment 785387 [details] [review]
https://github.com/mozilla/popcorn.webmaker.org/pull/162

A bunch of stuff seems to be missing from our session yesterday.  I guess this isn't ready for review?  Please fix and flag me again when it's updated.
Attachment #785387 - Flags: review?(schranz.m)
Attachment #785387 - Flags: review?(david.humphrey)
Attachment #785387 - Flags: review-
Ali, please put up a new attachment with the new/updated PR from our work yesterday when you have a chance, and flag me on that.
Comment on attachment 785387 [details] [review]
https://github.com/mozilla/popcorn.webmaker.org/pull/162

Sorry it was my mistake. This is a different PR now

https://github.com/mozilla/popcorn.webmaker.org/pull/162
Attachment #785387 - Attachment description: https://github.com/mozilla/popcorn.webmaker.org/pull/152 → https://github.com/mozilla/popcorn.webmaker.org/pull/162
Attachment #785387 - Flags: review- → review?(david.humphrey)

        Attachment #790865 -
        Flags: review?(david.humphrey)

    
  

        Attachment #785387 -
        Attachment is obsolete: true

    
    

    
  
Comment on attachment 790865 [details] [review]
https://github.com/mozilla/popcorn.webmaker.org/pull/162

My main concern here is that we sort out of the issues in the abide module to fix the things we found with getStrings.

See other comments in PR.

        Attachment #790865 -
        Flags: review?(david.humphrey) → review-

    
  

        Attachment #790865 -
        Flags: review- → review?(david.humphrey)

    
    

    
  
Comment on attachment 790865 [details] [review]
https://github.com/mozilla/popcorn.webmaker.org/pull/162

I've made numerous comments in the PR.  Please work through them all before flagging me again for review.  Hopefully we can do this in one more go round :)

        Attachment #790865 -
        Flags: review?(david.humphrey) → review-

    
  
Blocks: 907320

    
  

        Attachment #790865 -
        Flags: review- → review?(david.humphrey)

    
  
Blocks: 907360

        Attachment #790865 -
        Flags: review?(david.humphrey) → review-

    
  

        Attachment #790865 -
        Flags: review- → review?(david.humphrey)

    
    

    
  
Comment on attachment 790865 [details] [review]
https://github.com/mozilla/popcorn.webmaker.org/pull/162

r=me with the fixes I outlined in the last PR.  Make sure the Thai stuff comes out, that we add default_langauge to the middleware call, and anything else I flagged.

Really great work, Ali.  Thanks for not giving up on it--I know this wasn't an easy patch!

        Attachment #790865 -
        Flags: review?(david.humphrey) → review+

    
    

    
  
Nits:

- The title on our error page is now different. <title>Oops, you've found an error</title> vs <title>found an error</title>
- I can't save at all. It's not hitting up our /api/project or /api/publish endpoints.

    
    

    
  
Matt, with the second issue, is it stripping the /api/ from the URL?  Can you see what it's actually doing internally?

    
    

    
  
I have other comments in the PR.

Also, the editorhelper tooltips for dragging aren't being created at all now.

    
  

        Attachment #790865 -
        Flags: review+ → review?(david.humphrey)

    
    

    
  
Comment on attachment 790865 [details] [review]
https://github.com/mozilla/popcorn.webmaker.org/pull/162

A few nits in the PR, basically looks good to me.  r=me with those.

I want Matt to sign off on this too.  I think he has some CSS lint issues that need addressing.

Let's try to wrap this up and land today if at all possible.

        Attachment #790865 -
        Flags: review?(schranz.m)

        Attachment #790865 -
        Flags: review?(david.humphrey)

        Attachment #790865 -
        Flags: review+

    
    

    
  
Comment on attachment 790865 [details] [review]
https://github.com/mozilla/popcorn.webmaker.org/pull/162

R+ with my lint issues noted.

        Attachment #790865 -
        Flags: review?(schranz.m) → review+

    
  
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Attachment mime type: text/plain text/plain → text/x-github-pull-request text/x-github-pull-request

          You need to log in
          before you can comment on or make changes to this bug.
        






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: