Skip to content
This repository was archived by the owner on Jun 26, 2020. It is now read-only.

Added a special character info view #9

Merged
merged 4 commits into from
Jan 2, 2020
Merged

Added a special character info view #9

merged 4 commits into from
Jan 2, 2020

Conversation

pomek
Copy link
Member

@pomek pomek commented Dec 20, 2019

Suggested merge commit message (convention)

Feature: Added a special character info view. Closes ckeditor/ckeditor5#5817.


Additional information

Requires CSS: ckeditor/ckeditor5-theme-lark#260

Copy link
Contributor

@jodator jodator left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The feature works nice and the code is OK.

We only miss the docs.


import '../../theme/characterinfo.css';

export default class CharacterInfoView extends View {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add some docs for the class.

this.set( 'character', null );
this.set( 'name', null );

this.bind( 'code' ).to( this, 'character', char => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not so needed but maybe extract this to charcterToUnicode function as implementation is not needed here - only what happens in this binding.

},
children: [
{
text: bind.to( 'name', name => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

WDYT about making it compact?

bind.to( 'name', name => name ? name : '\u200B' ) // ZWSP to prevent vertical collapsing.

6 linies for this looks like too much :D

}
} );

tile.on( 'mouseover', () => {
this.fire( 'tileHover', { character, name } );
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing docs for the event.

@oleq oleq merged commit 5d36463 into master Jan 2, 2020
@oleq oleq deleted the i/5817 branch January 2, 2020 13:00
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Improve special character label styling
3 participants