Opened 10 years ago

Closed 10 years ago

#11911 closed Bug (fixed)

[PR#98] Setting of the "dir" attribute in the preloaded lang JS case

Reported by: Marek Lewandowski Owned by: Marek Lewandowski
Priority: Normal Milestone: CKEditor 4.4.1
Component: UI : Language Version:
Keywords: Cc:

Description

GitHub user akashmohapatra created a pull request for following issue:

If language dictionary is predefined in CKEDITOR.lang object, it will not set CKEDITOR.lang[ '<langId>' ].dir property.

  1. open AJAX sample with CKEditor (i.e. samples/ajax.html)
  2. execute following code in console:
    CKEDITOR.lang[ CKEDITOR.lang.detect() ] = { common: {} }
    
  3. Click "Create Editor" button to init first CKE instance
  4. execute following code in console
    CKEDITOR.lang[ CKEDITOR.lang.detect() ].dir
    

Expected result:
This statement should return a string: "ltr" (or "rtl" in case of using rtl lang)

Current result:
Variable is not assigned: undefined

Pull request can be found at GitHub.

Change History (6)

comment:1 Changed 10 years ago by Marek Lewandowski

Owner: set to Marek Lewandowski
Status: newassigned

comment:2 Changed 10 years ago by Marek Lewandowski

Status: assignedreview

I added few minor fixes to proposed solution, and removed unused variable in CKEDITOR.lang.

Pushed to t/11911 at dev and t/11911 at tests.

comment:3 Changed 10 years ago by Piotrek Koszuliński

Milestone: CKEditor 4.4.1
Status: reviewreview_failed
  1. There are missing spaces in closure in tests.
  2. I don't think we need to mock lang.detect - I commented that part out and it worked.
  3. We should write assertion messages as positive statements - what should be the desirable state.

The dev changes are ok, but you could squash the commit fixing code style into previous one.

Last edited 10 years ago by Piotrek Koszuliński (previous) (diff)

comment:4 Changed 10 years ago by Marek Lewandowski

Status: review_failedreview

Yea that's true, mocking was not needed in this case. I've removed it, fixed codestyle and squashed commits.

Rebased and force pushed both branches.

comment:5 Changed 10 years ago by Piotrek Koszuliński

Status: reviewreview_passed

Fix the message:

+                        assert.areEqual( 'ltr', CKEDITOR.lang.en.dir, 'lang.ar.dir' );

And remember to close the PR.

comment:6 Changed 10 years ago by Marek Lewandowski

Resolution: fixed
Status: review_passedclosed

Fixed with git:5b1b8ed65b (merged to master) at dev and 246c4aaef0 (merged to master) at tests.

Note: See TracTickets for help on using tickets.
© 2003 – 2022, CKSource sp. z o.o. sp.k. All rights reserved. | Terms of use | Privacy policy