Opened 5 years ago

Closed 5 years ago

#10862 closed Bug (fixed)

Placeholder plugin does not work any more

Reported by: Piotrek Koszuliński Owned by: Marek Lewandowski
Priority: Normal Milestone: CKEditor 4.3
Component: General Version: 4.3 Beta
Keywords: Cc:

Description (last modified by Piotrek Koszuliński)

Since non-editable elements are not processed by default rules placeholders are not serialized to [[xxx]] format.

However, since we have widgets we may want to rewrite placeholders with them. Should be quite simple.

Change History (9)

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

Description: modified (diff)
Status: newconfirmed

comment:2 Changed 5 years ago by Marek Lewandowski

Owner: set to Marek Lewandowski
Status: confirmedassigned

comment:3 Changed 5 years ago by Marek Lewandowski

Status: assignedreview

Fixed with git:c2e41c99689 on dev and 47224411 on tests. Branch t/10862.

Last edited 5 years ago by Marek Lewandowski (previous) (diff)

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

Status: reviewreview_failed

I pushed few fixes and improvements, but there are still some issues to be fixed.

What I did:

  • CKEDITOR.addCss should be executed only once.
  • cke_placeholder class is used to style placeholder - better backward compatibility and easier styling.
  • Removed defaults object.
  • Reverted one of language changes - button still uses lang.toolbar entry, however now it's "Placeholder" instead of "Insert Placeholder", because button also edits placehoders.

Whats need to be done:

  • Code of tests:
    • we do not wrap single statement inside if/for/else/while with {},
    • there's always space between } ),
    • this is incorrect: CKTESTER.test(\n{,
    • there should be no empty line after function/other block beginning,
    • we do not align comments inside doc strings,
    • description of an argument/property/everything else always starts with an uppercased letter and ends with a period,
    • assertDowncast should not read data from div, but from textarea - see CKTESTER.tools.getValueAsHtml,
    • there should be one empty line after method/property/class description and list of params/return value/etc,
    • Array#map is now available in IE8,
    • This if ( typeof expectedInstancesCount === undefined ) { is incorrect,
    • This typeof focusedWidgetIndex === 'number' is still incorrect - == is enough.
  • I'm unsure about changes in language files at all. Old language files were removed because new properties were defined in en.js, however I reverted more of those. Now we have only 3 changes:
    • invalidName was added which is an extended version of textMissing which was removed (BTW. the value of this property is incorrect - "Placeholder" and "the following"),
    • toolbar has been changed from "Insert Placeholder", to "Placeholder",
    • text was changed to name.

Wiktor & Fred - what do we do with languages? Do we try to keep them (and if yes - which?) or the changes are good?

comment:5 Changed 5 years ago by Wiktor Walc

Once a language has enough translations to be included in CKEditor core at all, we should keep as many language files for this language as possible.

It means that language files for plugins should be generally removed only if, after making modifications in them, there are no translated strings. Here, after changes, we have 50% of texts that remained, so there is no need to remove language files.

For the rest I believe we're okay, we changed the strings, so changing the properties for them is even better.

comment:6 Changed 5 years ago by Frederico Caldeira Knabben

Confirmed with Wiktor that we can proceed with the language changes in the way proposed in the branch (keeping just "en").

comment:7 Changed 5 years ago by Marek Lewandowski

Status: review_failedreview

@Reinmar - updated codestyle in tests branch t/10862 as in your guidelines with exception of assertDowncast

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

Status: reviewreview_passed

I pushed additional commit to tests fixing some minor code style issues.

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

Resolution: fixed
Status: review_passedclosed

Fixed on major with git:563c991 and 03ab76b on tests.

One more successful widget implementation :) Great job.

Note: See TracTickets for help on using tickets.
© 2003 – 2019 CKSource – Frederico Knabben. All rights reserved. | Terms of use | Privacy policy