Opened 3 years ago

Closed 3 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 3 years ago by Marek Lewandowski

Owner: set to Marek Lewandowski
Status: newassigned

comment:2 Changed 3 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 3 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 3 years ago by Piotrek Koszuliński (previous) (diff)

comment:4 Changed 3 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 3 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 3 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 – 2017 CKSource – Frederico Knabben. All rights reserved. | Terms of use | Privacy policy