Opened 7 years ago

Closed 7 years ago

#9580 closed Bug (fixed)

Fix samples to make them 100% valid for all possible configurations

Reported by: Wiktor Walc Owned by: Piotrek Koszuliński
Priority: Normal Milestone: CKEditor 4.0
Component: General Version: 4.0 Beta
Keywords: Cc:

Description

1) samples/api.html has "Execute link" and "Execute bold" buttons. The buttons should be hidden if plugins are not available.

2) apidialog.html sample depends on the Link dialog, but should it really land in the plugins/link folder because of that? Maybe we need a more generic sample for dialogs.

3) 'samples/uicolor.html' sample right now needs the uiColor plugin to display color picker. The little problem is that we have two things here:

  • the skin that can be customized even without the uicolor plugin, which I believe deserves a simplified sample
  • and the relatively big plugin, which provides the color picker dialog

There might be more samples to fix, I just mentioned the one that I've quickly noticed

Change History (16)

comment:1 Changed 7 years ago by Wiktor Walc

4) samples/fullpage.html looks like similar case to samples/uicolor.html. Full page mode is supported by CKEditor, but the docprops is not actually required for that.

comment:2 Changed 7 years ago by Wiktor Walc

5) samples/enterkey.html requires the enterkey plugin (?), so should be probably moved to ckeditor-dev/plugins/enterkey/samples folder, and marked with meta tags as "Advanced sample", just like plugins/divarea/samples/divarea.html

comment:3 Changed 7 years ago by Garry Yao

The enter mode feature is not an affiliation of the enterkey plugin, it is instead an integrated part of the editor content handling, e.g. it affects data loading, so this sample is not subjected to be moved IMO.

comment:4 Changed 7 years ago by Garry Yao

Is it a problem to keep plugin specific demonstration inside of basic samples (uicolor and fullpage cases)? I think it make sense as these are highly related to the feature, and it safely use the "extraPlugin" config.

comment:5 Changed 7 years ago by Garry Yao

Owner: set to Garry Yao
Status: newreview

Opened dev branch t/9580 for some sample pages.

comment:6 Changed 7 years ago by Garry Yao

Status: reviewreview_failed

Wiktor has just pointed out that plugin specific samples must be moved out of core samples, as the --skip-omitted-in-build-config (-s) param (used by default in the online builder) will keep only selected plugins.

comment:7 Changed 7 years ago by Garry Yao

Status: review_failedreview

Moved all plugins dependent samples out of core, so released sample will not depend on any other plugin.

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

I pushed rebased t/9580.

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

Status: reviewreview_failed
  1. Toolbar conf sample is in two different places in dev and release versions.
  2. There is number of coding style issues introduced (I can help in finding them).
  3. There's no "Append to" sample for div area. Even I don't know when which creator is used.
  4. Breadcrumb in UIColor plugin sample has outdated URL. Perhaps other samples moved to plugins folder too.
  5. Why CKEDITOR#pluginsLoaded plugin has been added? There already is editor#pluginsLoaded and it's even documented :). So I guess that this is a mistake.

comment:10 Changed 7 years ago by Garry Yao

Status: review_failedreview
  1. Builder doesn't not allow plugin samples to stay in the "Basic Customization" group, perhaps it's a issue, for now I'd have it as "Advanced Samples"
  2. Let's handle them at last.
  3. Append to sample for divarea plugin can be missed IMO.
  4. Fixed
  5. CKEDITOR.dialog.add is a global method which looks wrong to be called inside of an instance specific event handler, but I can revert this change.

comment:11 Changed 7 years ago by Garry Yao

Additionally I moved two other samples (outputxhtml & outputforflash) to plugins as well, considering they actually depends on the htmlwriter plugin.

comment:12 Changed 7 years ago by Piotrek Koszuliński

Status: reviewreview_failed
  1. I pushed commit adding uicolor plugin sample to dev index.
  2. Flash sample does not work any more.
  3. Submit in tabindex sample sends empty form.

comment:13 Changed 7 years ago by Piotrek Koszuliński

I pushed few commits improving coding style in samples. Most of these issues werent' introduced by this branch, but I fixed them reviewing this branch, so they are included.

comment:14 Changed 7 years ago by Piotrek Koszuliński

Owner: changed from Garry Yao to Piotrek Koszuliński
Status: review_failedassigned

comment:15 Changed 7 years ago by Piotrek Koszuliński

Status: assignedreview

comment:16 Changed 7 years ago by Piotrek Koszuliński

Resolution: fixed
Status: reviewclosed

Squashed by Garry & masterised by me https://github.com/ckeditor/ckeditor-dev/commit/4efab72

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