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

New positions added to drop down panel positions. #8670

Merged
merged 18 commits into from
Jan 5, 2021

Conversation

magda-chrzescian
Copy link
Contributor

@magda-chrzescian magda-chrzescian commented Dec 15, 2020

Suggested merge commit message (convention)

Bug (ui): The dropdown should always take an accurate position to be fully visible. Closes #7700. Closes #8669.

@magda-chrzescian magda-chrzescian marked this pull request as draft December 15, 2020 16:03
@magda-chrzescian magda-chrzescian marked this pull request as ready for review December 15, 2020 16:03
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.

The solution is correct. Although to see the results we need to wait for #8669 to resolve.

We could also try defining more south positions like the BalloonPanelView to give the algorithm more choices. But maybe let's see how #8669 turns out first.

@magda-chrzescian magda-chrzescian changed the title South position added to drop down panel positions. New positions added to drop down panel positions. Dec 16, 2020
@magda-chrzescian
Copy link
Contributor Author

magda-chrzescian commented Dec 16, 2020

I added 4 additional positions, so the "step" in positioning is now 25% of the view width. I also set drop-down max-width to 75vw so all of the potential cases should be fully covered.

I think that a test checking if the positions of rendered panels are equal to these calculated would be useful.

I'd like to resolve #8669 also in this PR to have a full preview before merging.

/* https://github.com/ckeditor/ckeditor5/issues/5586 */
width: max-content;
max-width: var(--ck-toolbar-dropdown-max-width);
.ck.ck-toolbar-dropdown > .ck-dropdown__panel {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

FYI @oleq I specified this selector because it affected the size of dropdowns nested in the toolbar dropdown (it was messing up with the sizes of the panels).

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.

Take-aways from yesterday's call:

  • Let's prioritize (S|N)W and (S|N)E positions to avoid using intermediate ones when there's no such need, for instance here:
    image
  • Let's handle Special characters grid exceeds drop-down size on small screen. #8669 in the same PR. Although not exactly the same issue, these are related, and handling them both will certainly help me verify whether everything works as expected.

@oleq oleq merged commit 52ce85b into master Jan 5, 2021
@oleq oleq deleted the i/7700-invisible-dropdown branch January 5, 2021 13:23
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.

Special characters grid exceeds drop-down size on small screen. Special characters dropdown is cut on mobile
2 participants