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

i/6536: Implemented the select all feature #1

Merged
merged 10 commits into from
Apr 17, 2020
Merged

i/6536: Implemented the select all feature #1

merged 10 commits into from
Apr 17, 2020

Conversation

oleq
Copy link
Member

@oleq oleq commented Apr 14, 2020

Feature: Implemented the select all feature. Closes ckeditor/ckeditor5#6536.


To be merged together with:


Q: Do we need a feature guide? That would be a very short one but maybe it's worth adding. cc @Reinmar


FYI: The feature brings a UI button, which looks as follows:

image

@jodator jodator self-requested a review April 15, 2020 09:35
@jodator jodator self-assigned this Apr 15, 2020
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.

Basically I see room for improvement in tests. Maybe it is worth testing it with table > image also?

I'm confused with the keystrokes vs listenTo( viewDocument ) usage as I don't see difference in them and personally I would use editor.keystrokes. So this is question to @Reinmar alongside @oleq ones.

ps.: besides that LGTM 👍 for merging.

} );
} );

describe( 'execute()', () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think that we are safe but WDYT about adding a table test?

This would be a scenario for nested-nested editable.

Copy link
Member Author

Choose a reason for hiding this comment

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

Nested editable is tested in the image caption.

Copy link
Contributor

@jodator jodator Apr 16, 2020

Choose a reason for hiding this comment

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

Yes, but a nested editable in a nested editable case - to be sure that we select inner, not the outer nested editable.

Copy link
Member Author

Choose a reason for hiding this comment

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

Alright, makes sense 👍

@oleq
Copy link
Member Author

oleq commented Apr 16, 2020

Ready for another round of review.

@oleq
Copy link
Member Author

oleq commented Apr 16, 2020

An afterthought: do you think it would be a nice UX if pressing Ctrl+A in a nested editable which content is already selected (double Ctrl+A) selected the closest limit ancestor?

For instance, root->table->cell->image width caption. Double Ctrl+A in the caption would select the entire content of a cell. And a double Ctrl+A would select the entire root.

@Reinmar
Copy link
Member

Reinmar commented Apr 16, 2020

Q: Do we need a feature guide? That would be a very short one but maybe it's worth adding. cc @Reinmar

👍  because of the "select all"  button. Otherwise, it will be extremely hard to find.

@Reinmar
Copy link
Member

Reinmar commented Apr 16, 2020

An afterthought: do you think it would be a nice UX if pressing Ctrl+A in a nested editable which content is already selected (double Ctrl+A) selected the closest limit ancestor?

Just to make sure that I understand correctly. In the following case:

For instance, root->table->cell->image->caption

  1. Ctrl+A to select image caption
  2. Ctrl+A to select the image
  3. Ctrl+A to select the cell content (if the image is not already its only content)
  4. Ctrl+A to select the entire table
  5. Ctrl+A to select the entire root?

Sounds great.

@oleq
Copy link
Member Author

oleq commented Apr 16, 2020

Honestly, I’d skip 2 and 4. They don’t feel like select all but widget features.

@Reinmar
Copy link
Member

Reinmar commented Apr 16, 2020

BTW, the way to implement that behaviour of selecting the next "thing" on Ctrl+A press when entire content of a limit element is selected is to look for the next object element. That'll be more semantically correct than looking for another limit element.

All objects are by default limit elements, so looking for another limit would work fine. But conceptually we're looking then for another object.

See #1 (comment).

@Reinmar
Copy link
Member

Reinmar commented Apr 16, 2020

Honestly, I’d skip 2 and 4. They don’t feel like select all but widget features.

Could be this way too 👍

This would also invalidate my previous comment.

src/selectallui.js Outdated Show resolved Hide resolved
oleq and others added 3 commits April 16, 2020 16:08
@oleq
Copy link
Member Author

oleq commented Apr 16, 2020

👍  because of the "select all"  button. Otherwise, it will be extremely hard to find.

Created.

Sounds great.

I created a follow-up: ckeditor/ckeditor5#6621. I think this could be a great intro ticket.


Ready for another round of review :)

@Reinmar Reinmar merged commit f0c49d3 into master Apr 17, 2020
@Reinmar Reinmar deleted the i/6536 branch April 17, 2020 14:49
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.

Handle Ctrl/Cmd+A when a widget is at the beginning or end of the content
3 participants