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

T/40: Implement the selection handler #43

Merged
merged 10 commits into from
Jun 27, 2018
Merged

T/40: Implement the selection handler #43

merged 10 commits into from
Jun 27, 2018

Conversation

jodator
Copy link
Contributor

@jodator jodator commented Jun 20, 2018

Suggested merge commit message (convention)

Feature: Implement the selection handler. Closes ckeditor/ckeditor5#4586.


Additional information

@jodator jodator requested a review from oleq June 20, 2018 15:19
@jodator
Copy link
Contributor Author

jodator commented Jun 21, 2018

This PR may need some additional work: ckeditor/ckeditor5#974 (comment). It might be that import ... .svg is a no-go for the utils.js and we might need to think how to structure this better.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling 859f5af on t/40 into 70e01ea on master.

@oleq
Copy link
Member

oleq commented Jun 25, 2018

We need more spacing around the widget (see https://ckeditor.com/ckeditor-4/) because it's hard to use the handler

kapture 2018-06-25 at 14 25 13

@oleq
Copy link
Member

oleq commented Jun 25, 2018

I just found out that we were able to implement it like this in v4 because <figure> has another <div class="cke_widget_wrapper"> wrapper :/ The widget outline and handler "belong" (stick) to the <figure> but they react on the wrapper when it comes to :hover.

TBH I don't like the UX proposed in this PR but OTOH I'm not sure we can affort another wrapper. WDYT @jodator @Reinmar @dkonopka?

@oleq
Copy link
Member

oleq commented Jun 25, 2018

Also, the handler should be closed to the widget:
localhost_8125_ckeditor5-table_tests_manual_table html 1

@oleq
Copy link
Member

oleq commented Jun 25, 2018

I'm also worried about the contrast in this situation:

image

Maybe coloring the handler to reflect the border wasn't the best idea after all? In v4, the handler always looks the same

image

Maybe we should reverse the color in this state?

image

@dkonopka
Copy link
Contributor

dkonopka commented Jun 26, 2018

Reversing color is a good idea 👍

Would be nice if we could show an active/inactive state of widget selection not only via outline color change (because first time user doesn't see a difference between yellow/blue) but in icon animation (or completely change it in this case). WDYT?

Fast proposal (scale svg: 0.85 if active)

selectionhandler

The other thing is the size of this selection handler - are you comfortable to catch it? How about making invisible right 10px padding or enlarge width of this button?

selectionarea

oleq added 2 commits June 26, 2018 13:01
…standards. Moved widget selection handler styles to ckeditor5-theme-lark. Implemented a 2-state handler icon.
Copy link
Member

@oleq oleq left a comment

Choose a reason for hiding this comment

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

Minor naming issue

src/utils.js Outdated
@@ -61,6 +66,10 @@ export function toWidget( element, writer, options = {} ) {
setLabel( element, options.label, writer );
}

if ( options.addSelectionHandler ) {
Copy link
Member

Choose a reason for hiding this comment

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

hasSelectionHandler, I'd say.

@oleq
Copy link
Member

oleq commented Jun 26, 2018

Just for the record:

src/utils.js Outdated
@@ -184,7 +184,7 @@ function getFillerOffset() {
//
// @param {module:engine/view/editableelement~EditableElement}
// @param {module:engine/view/writer~Writer} writer
function addSelectionHandler( editable, writer ) {
function hasSelectionHandler( editable, writer ) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

shouldn't this stay as addSelectionHandler() method?

Copy link
Member

Choose a reason for hiding this comment

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

My bad. Search/replace. Fixing :/

@oleq oleq merged commit bbf9298 into master Jun 27, 2018
@oleq oleq deleted the t/40 branch June 27, 2018 08:25
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.

Implement the selection handler
4 participants