Opened 11 years ago
Closed 11 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 )
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 11 years ago by
Description: | modified (diff) |
---|---|
Status: | new → confirmed |
comment:2 Changed 11 years ago by
Owner: | set to Marek Lewandowski |
---|---|
Status: | confirmed → assigned |
comment:3 Changed 11 years ago by
Status: | assigned → review |
---|
comment:4 Changed 11 years ago by
Status: | review → review_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 - seeCKTESTER.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.
- we do not wrap single statement inside
- 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 oftextMissing
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 toname
.
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 11 years ago by
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 11 years ago by
Confirmed with Wiktor that we can proceed with the language changes in the way proposed in the branch (keeping just "en").
comment:7 Changed 11 years ago by
Status: | review_failed → review |
---|
@Reinmar - updated codestyle in tests branch t/10862 as in your guidelines with exception of assertDowncast
comment:8 Changed 11 years ago by
Status: | review → review_passed |
---|
I pushed additional commit to tests fixing some minor code style issues.
comment:9 Changed 11 years ago by
Resolution: | → fixed |
---|---|
Status: | review_passed → closed |
Fixed on major with git:563c991 and 03ab76b on tests.
One more successful widget implementation :) Great job.
Fixed with git:c2e41c99689 on dev and 47224411 on tests. Branch t/10862.