This repository has been archived by the owner on Jun 26, 2020. It is now read-only.
T/1: Initial implementation of Highlight editing without UI. #2
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…ulation on refresh.
jodator
changed the title
szymonkups
suggested changes
Nov 9, 2017
src/highlightcommand.js
Outdated
| * @observable | ||
| * @readonly | ||
| * @member {undefined|String} HighlightCommand#value | ||
| */ |
There was a problem hiding this comment.
This gives some docs errors - it should be moved probably to the class constructor or use full module name.
src/highlightcommand.js
Outdated
| import Command from '@ckeditor/ckeditor5-core/src/command'; | ||
|
|
||
| /** | ||
| * The highlight command. It is used by the {@link module:highlight/highlight~HighlightEditing highlight feature} |
There was a problem hiding this comment.
Gives some docs errors, probably wrong module in @link.
src/highlightediting.js
Outdated
| const data = editor.data; | ||
| const editing = editor.editing; | ||
|
|
||
| editor.config.define( 'highlight', [ |
There was a problem hiding this comment.
Shouldn't this be defined inside the class constructor?
src/highlightediting.js
Outdated
| .for( data.viewToModel ) | ||
| .fromElement( 'mark' ) | ||
| .toAttribute( viewElement => { | ||
| const viewClassNames = [ ...viewElement.getClassNames() ]; |
There was a problem hiding this comment.
This function could be simplified to:
const viewClassNames = [ ...viewElement.getClassNames() ];
for( const className of viewClassNames ) {
if ( configuredClasses.indexOf( className ) > -1 ) {
return { key: 'highlight', value: className };
}
}
There was a problem hiding this comment.
Writing:
for( const className of viewElement.getClassNames() ) {}We'd take advantage of generators and save extra ms ;)
There was a problem hiding this comment.
@ma2ciek 👏 yeah... I've disabled thinking apparently :D
src/highlightediting.js
Outdated
| * @property {String} title The user-readable title of the option. | ||
| * @property {String} color Color used for highlighter. Should be coherent with CSS class definition. | ||
| * @property {'marker'|'pen'} type The type of highlighter. Either "marker" - will use #color as background name | ||
| * of the view element that will be used to represent the model element in the view. |
There was a problem hiding this comment.
Looks like pen option description is missing.
src/highlightediting.js
Outdated
| * | ||
| * Read more in {@link module:highlight/highlightediting~HighlightEditingConfig}. | ||
| * | ||
| * @member {module:highlight/highlightediting~HighlightEditingConfig} module:core/editor/editorconfig~EditorConfig#alignment |
src/highlightediting.js
Outdated
| ] ); | ||
|
|
||
| // Allow highlight attribute on all elements | ||
| editor.document.schema.allow( { name: '$inline', attributes: 'highlight', inside: '$block' } ); |
There was a problem hiding this comment.
Basic styles contains this workaround. It probably should be included here as well.
szymonkups
approved these changes
Nov 10, 2017
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Suggested merge commit message (convention)
Feature: Initial implementation of highlight editing. Closes ckeditor/ckeditor5#2597.
Additional information
This PR comes with changes in other packages branches:
Merge commit message for related branches:
Feature: Introduce ckeditor5-highlight package.Misc
Markerswere used in view - only<mark>elements.