Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Widget - hide selection handle for nested widget #10556

Merged
merged 4 commits into from
Sep 28, 2021

Conversation

EwelinaD
Copy link
Contributor

@EwelinaD EwelinaD commented Sep 19, 2021

Suggested merge commit message (convention)

Fix (table): Selected nested widget in table selection has no longer outline and selection handle. Closes #9491.


Additional information

Fix is based on assumption that there are multiple ranges during data selection in table.

Copy link
Contributor

@dawidurbanski dawidurbanski left a comment

Choose a reason for hiding this comment

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

I made one comment/suggestion to consider.

Also I'm pretty sure we need to add a test to cover scenario with multiple ranges.

packages/ckeditor5-widget/src/widget.js Outdated Show resolved Hide resolved
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.

Let's circle back a little bit. There's a decision concerning UX we must make before we jump to writing code. I'm moving the discussion to the original issue #9491 (comment) where I listed possible scenarios.

@EwelinaD
Copy link
Contributor Author

I've just pushed suggested changes to tableselection.css.

@oleq
Copy link
Member

oleq commented Sep 24, 2021

@ckeditor/qa-team Can you give us a hand, please? 🙏 You can find inspiration in comments #9491 🙃

@FilipTokarski
Copy link
Member

Not sure if important.
I found this behaviour on master - #10600. I wouldn't mind about this, but in the context of this PR - apart from flashing border, the widget handler shows up for a split second as well, but I guess it shouldn't be here:

0_table1.mp4

@oleq
Copy link
Member

oleq commented Sep 24, 2021

Not sure if important.
I found this behaviour on master - #10600. I wouldn't mind about this, but in the context of this PR - apart from flashing border, the widget handler shows up for a split second as well, but I guess it shouldn't be here:

Let's address it in a separate issue.

@oleq
Copy link
Member

oleq commented Sep 24, 2021

@dawidurbanski I don't feel like reviewing a solution I suggested myself. Can you do the review please?

BTW: What's going on with the CI?

@EwelinaD
Copy link
Contributor Author

Hmm, build details:
The following packages did not provide required code coverage: engine
But there were no changes in the engine package

@FilipTokarski
Copy link
Member

Apart from the comment above, I didn't find anything else.

@oleq
Copy link
Member

oleq commented Sep 27, 2021

The following packages did not provide required code coverage: engine

There's something wrong with the CI then. Don't worry.

@pomek
Copy link
Member

pomek commented Sep 27, 2021

WARN: 'Detected console.warn():', 'Selection change observer detected an infinite rendering loop.'

It decreases the CC. Restarting a build usually helps.

@dawidurbanski
Copy link
Contributor

@dawidurbanski I don't feel like reviewing a solution I suggested myself. Can you do the review please?

@oleq done

@EwelinaD I requested one last change. Thanks!

Copy link
Contributor

@dawidurbanski dawidurbanski left a comment

Choose a reason for hiding this comment

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

LGTM 👍🏻

@oleq oleq merged commit 80e520d into master Sep 28, 2021
@oleq oleq deleted the ck/9491-nested-table-selection branch September 28, 2021 07:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

The nested widget selection handle is visible while the outer table cells are selected
6 participants