Change to core/skin.js to enable setting both background pos_horz and pos_vert values for icon images #34
Conversation
As discussed at: http://ckeditor.com/forums/CKEditor/Considering-making-a-change-via-GitHub-wanted-sanity-check... Made a few changes to: CKEDITOR.skins.addIcon() added a fourth parameter, and renamed the third. These parameters: offset_vert[3:Number] offset_horz[4:Number] ...allow defining a specific icon image file, and allows specifying the background-position offset for both x and y positions. I also removed the "check" not to override any existing value, as a call to addIcon should always allow overriding a previous value. I also modified the call to: CKEDITOR.skin.getIconStyle() such that the background-position is now populated with any pos_horz pos_vert values defined with a call to addIcon. Specifying the offset_horz after the offset_vert breaks the normal x,y nomenclature but it maintains backwards compatiblity of all plugins that currently only specity an offset_vert value. My first commit, but reach out at zac@zacwolf.com if you have any questions/concerns.
| name = name.toLowerCase(); | ||
| if ( !this.icons[ name ] ) { | ||
| //if ( !this.icons[ name ] ) {//Removed, as it should always allow a call to override |
There was a problem hiding this comment.
I commented out this check, because the way the build process works is that the CKEDITOR.skins.icons[] array pretty much becomes immutable for those plugins built into ckeditor by ckbuilder.jar. A call to this this method should always expect to override any value that is already set. This does bring up one issue where "non-built-in" plugins that are included via ".extraPlugins:" are currently running AFTER skin.js from a custom skin, which means the plugin overrides any value set by skin.js. I'm going to implement a separate commit for that, as skin.js should always run "last" so that that it skin's skin.js becomes definitive.
|
Please do not mix two different issues together. Also, hoping that it's not your last pull request :) let me write, that an ideal way of dealing with this kind of issues would be creating a ticket for each separate problem @ http://dev.ckeditor.com/ It's because the second issue with the order of applying icons is something which should be first understood correctly to eventually make any changes. |
Ticket #10206 specify additional pos_horz when calling the addIcon from a skin or plugin. Useful when using pre-existing sprite files that may not be laid out in a single column of sprites.
This reverts commit fd4f95e.
This reverts commit 2e76d00.
Via Ticket #10206, address the need to specify a y/horizontal offset when specifying an icon to CKEditor.skins.addIcon. That allows using existing corporate colateral image sprites that may not be laid out in a single column. when defining a new skin or plugin. Adding the offset_horz after the offset_vert breaks the normal X before Y paradigm, but doing it this way maintains backwards compability with all existing source for plugins/skins. Sorry for the previous commits/reverts. Still new to github.
This reverts commit 6999de5.
As part of commit 6999de5 I modified the existing offset property in the addIcon method, but it would appear that a similar change would have to be made in ckbuilder.jar because after a build and load CKEDITOR.skin.icons contains icons objects that only had an "offset" value, and not the new "offset_vert" value that I had created. I reverted back to using the "offset" object property, such that calls to getIconStyle() would correctly handle those values registered in CKEDITOR.skins.icons[] during "building" by ckbuilder.jar. Without an additional "override" property, that allows non-"built" skins that are loaded via: extraPlugins:'xyz', skin:'albani' ...at runtime, this change may seem pointless, but one step at a time, as I'm submitting another change to add the "override" property to allow runtime loading of a new skin that wasn't "built" into ckeditor.js
This change adds an additional "overrride" property to
CKEDITOR.skins.addIcon() [core/skins.js]
The property would be use to override the current check to see if an
existing object is stored in the CKEDITOR.skins.icons array, prior to
allowing a change to be made.
The use case for this is to enable "dropin non-built skins". Skins that
are initialized at "run-time" via the config.js or via config:
initialization property: config ex:
jQuery("#id").ckeditor({skin:'myskin'});
The "skins" property causes ckeditor to load the skins/myskin/skin.js
after ckeditor.js is initailized, thus allows such a skin to "override"
the values set by the skin that was used during the "build" of
ckeditor.js, via adding calls to addIcon in the skin.js file.
|
OK, still new to github, so I wasn't sure how the whole revert vs. rollback was implemented , so I committed a revert of my original commit via (2e76d00). But, I then mistakenly committed 6789b1d, so I used another revert, 31cada0, to roll that back as well. I then committed 6999de5, which contains the changes for pos_vert,pos_horz, with the pre-existing "not exists" clause in place, but after testing after a build via ckbuilder.jar it would seem that ckbuilder.jar is setting icon objects that specifically reference the "offset" property (vs the offset_vert property that I introduced), so I reverted that back via f935630. I then introduced the new method parameter (leaving all previous parameters named as they were). That was comit 000198b, but without the "override" property/logic this is a useless "fix", so I went ahead and committed that change in 50200d7. I opened two tickets: 10206 and 10208 trying to keep them separated as you suggested but one is pretty useless without the other, so put this both in this single pull request. Here is the example where it is of use. I've created a custom skin. I load that skin via a config wherein I specify skin={nameofmyskin}. Making this a "run-time skin" (vs one that was built into ckeditor.js via ckbuilder.jar). Why I don't want to "build" it is because it is just a single piece of a larger website, and I'm leveraging image files that aren't available at "build time", only run-time. Doing it this way allows me to drop-in new "default full builds" of CKEditor as they come available, then just dump my skin directory into the skins folder. At init, my skin's skin.js is run, where I have a series of CKEDITOR.skins.addIcon() calls. Since these calls reference icons from existing corporate sprites image (not laid out in a single column) I need to be able to specify both a pos-x and pos-y, and since this is runner after any "built" skin runs, it needs the ability to "override" values set by ckbuilder.jar. This final way should mean that there is no impact to the current code, just additional functionality that can be used (or not). THANKS! |
|
Hey, know you guys are all probably pretty busy with the 10th anniversary festivities, but being new to GitHub and this project; I was wondering, what is the usual turn-around for processing pull requests? I don't mean to rush in any way! I just want to communicate back to folks on my side the timeline. I've created a custom skin/plugin for my project, but it needs this change to function. Part of why I got involved is because co-workers had actually modified CKEditor core code to implement UI changes, vs using the skin/plugins framework [I know! Right?]. Basically tying us to a specific older version of CKEditor. With this pull request in place in my code I've been been able to show them that it's possible to use the existing framework that allows dropins of new CKEditor4 versions without losing our customizations. |
|
My recommendation is not to count on us including anything in the official branches. The point is that you can't predict the point in time when your code will be merged on master and not even if it'll be ever reach it. It may happen that the core team sees this as a unnecessary feature for our broad user base and it will never reach master. This is totally fine though and git and GitHub help you handling this internal requirement that you have. The right way for it is having you maintain your fork of CKEditor and constantly merging from the original branches (master, major, stable, latest or any of the release/ branches). Over your fork, you can maintain several branches with the different features and fixes you want to bring into, having all of them merged back into your internal "my_ckeditor" (whatever name) branch. Then when you feel one of your feature/fix branches are good to be contributed back, simply open a pull request for it. Still you should not impose the branches to be accepted, but simply open the possibility for it to happen. As the CKEditor development goes ahead in parallel with your fork, you simply keep synching the repos, so you'll always have the latest CKEditor merged with the code that makes it just perfect for your very specific case. That's open source and that flexibility makes it great. Have fun! |
|
Ah, that is unfortunate, because the whole point of my pull request is to get away from "one-off" code changes and managing merge synchronization forever. |
Update plugin.js with Italian Language
Update plugin.js with Italian Language
As discussed at:
http://ckeditor.com/forums/CKEditor/Considering-making-a-change-via-GitHub-wanted-sanity-check...
Made a few changes to:
CKEDITOR.skins.addIcon() added a fourth parameter, and renamed the
third. These parameters:
offset_vert[3:Number]
offset_horz[4:Number]
...allow defining a specific icon image file, and allows specifying the
background-position offset for both x and y positions. I also removed
the "check" not to override any existing value, as a call to addIcon
should always allow overriding a previous value.
I also modified the call to: CKEDITOR.skin.getIconStyle() such that the
background-position is now populated with any pos_horz pos_vert values
defined with a call to addIcon.
Specifying the offset_horz after the offset_vert breaks the normal x,y
nomenclature but it maintains backwards compatiblity of all plugins that
currently only specity an offset_vert value.
My first commit, but reach out at zac@zacwolf.com if you have any
questions/concerns.