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

Do not show back button if balloon was not opened using another balloon. #17562

Conversation

Mati365
Copy link
Member

@Mati365 Mati365 commented Nov 29, 2024

Suggested merge commit message (convention)

Internal (bookmark, link, image): Do not show back button if balloon was not opened using another balloon.
Fix (image): No longer crash editor when using image resize buttons in toolbar.

Additional information

Take a look at back button arrows.

Before

before-hide-arrows-2024-12-02_11.56.34.mp4

After

after-hide-arrows-2024-12-02_11.55.08.mp4

@Mati365 Mati365 force-pushed the ck/epic/17230-change-back-button-visibility branch 2 times, most recently from f5ab382 to 0f4631d Compare December 2, 2024 10:16
@Mati365 Mati365 marked this pull request as ready for review December 2, 2024 10:33
@Witoso
Copy link
Member

Witoso commented Dec 2, 2024

This should be implemented for all balloons

  • text alternative
  • custom image resize,

as someone can add them to the main toolbar, not balloon.

To test: behavior in the block toolbar / menu bar. (no back)

@Mati365
Copy link
Member Author

Mati365 commented Dec 2, 2024

Hm, I implemented that this way, but I thought that it's impossible to set custom size using menubar / or toolbar for such features. I'll check them, last time there were issues with them regarding the toolbar placement.

@Mati365 Mati365 force-pushed the ck/epic/17230-change-back-button-visibility branch from 72ad12f to ab85639 Compare December 3, 2024 06:13
@Mati365 Mati365 force-pushed the ck/epic/17230-change-back-button-visibility branch from ab85639 to fed492f Compare December 3, 2024 06:13
@Mati365
Copy link
Member Author

Mati365 commented Dec 3, 2024

@Witoso I checked both places, and I was unable to open image resize / image text alternative widget balloons without having image selected. It's crucial because selection of image triggers showing the default widget toolbar, which is being hidden after stacking another balloon on top of it (and after pressing back in these stacked balloons, it's shown again).

Copy link
Contributor

@pszczesniak pszczesniak left a comment

Choose a reason for hiding this comment

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

One thing to consider.

packages/ckeditor5-bookmark/src/bookmarkui.ts Outdated Show resolved Hide resolved
packages/ckeditor5-link/src/linkui.ts Outdated Show resolved Hide resolved
Copy link
Contributor

@niegowski niegowski left a comment

Choose a reason for hiding this comment

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

There is a strange behavior when you put caret in a link (link toolbar appears) and you hit Cmd+K. There is no back button but it should be there.

@Mati365
Copy link
Member Author

Mati365 commented Dec 3, 2024

Will check that.

edit: 🤦 It looks like toolbar button intercepts CTRL+K so technically, it's opened via toolbar.

@Mati365
Copy link
Member Author

Mati365 commented Dec 3, 2024

@niegowski I simplified logic, it should work fine after modification.

@Mati365 Mati365 merged commit 0cfd778 into ck/epic/17230-linking-experience Dec 3, 2024
7 checks passed
@Mati365 Mati365 deleted the ck/epic/17230-change-back-button-visibility branch December 3, 2024 12: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.

4 participants