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

t/ckeditor5-table/24: Implemented list component separators #407

Merged
merged 2 commits into from
Jun 13, 2018

Conversation

oleq
Copy link
Member

@oleq oleq commented Jun 12, 2018

Suggested merge commit message (convention)

Feature: Implemented list component separators (see ckeditor/ckeditor5#3178).


Additional information

Part of ckeditor/ckeditor5-table#46.

super( locale );

this.setTemplate( {
tag: 'span',
Copy link
Contributor

Choose a reason for hiding this comment

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

I have doubts that it will be a valid HTML? The list items are <li> elements inside <ul> (right now they are not buttons). If we plan to switch list to set of to buttons/other elmeents then it's probably OK.

ps.: I've checked this in HTML validator:

Error: Element span not allowed as child of element ul in this context. (Suppressing further errors from this subtree.)

From line 8, column 308; to line 8, column 343

 left</li><span class="ck ck-list__separator"></span

Contexts in which element span may be used:
    Where phrasing content is expected.
Content model for element ul:
    Zero or more li and script-supporting elements.

Copy link
Member Author

@oleq oleq Jun 12, 2018

Choose a reason for hiding this comment

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

You're right. I copy-pasted it from the toolbar component without much thinking. In fact... we can get rid of the ListSeparatorView and use ListItemView with an additional CSS class instead.

Copy link
Member Author

Choose a reason for hiding this comment

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

I fixed this issue. It turned out we cannot use the ListItemView for that purpose because it implements the focusable interface and the separator must be transparent for the keyboard navigation.

Copy link
Contributor

Choose a reason for hiding this comment

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

I find this clean to have ListSeparatorView instead of reausing ListItemView so it gonna be OK.

@jodator jodator merged commit 0808a8c into master Jun 13, 2018
@jodator jodator deleted the t/ckeditor5-table/24 branch June 13, 2018 10:19
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.

2 participants