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

Added support for RTL in html-embed. #8535

Merged
merged 3 commits into from
Nov 30, 2020
Merged

Added support for RTL in html-embed. #8535

merged 3 commits into from
Nov 30, 2020

Conversation

niegowski
Copy link
Contributor

@niegowski niegowski commented Nov 26, 2020

Suggested merge commit message (convention)

Fix (html-embed): HTML embed editing UI should not be broken when the editor uses an RTL language. Closes #8335.

Fix (theme-lark): HTML embed editing UI should not be broken when the editor uses an RTL language. 

Feature (ui): Added support for a new south-east position of the TooltipView.

Feature (theme-lark): Added styles for a new south-east position of the TooltipView


Additional information

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.

There's also one thing not covered by this PR: These styles should not work in RTL because the embed label is on the other side and we don't need to compensate for it.

Actual:

image

Expected:

image

@@ -118,3 +122,17 @@
}
}

.ck-content[dir="rtl"] .ck-widget.raw-html-embed {
Copy link
Member

Choose a reason for hiding this comment

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

These are not content styles and they should not start with .ck-content. Both the label and the buttons wrapper are not in the editor output data and the styles should not be listed in the guide.

I think this should be done using @mixin ck-dir rtl { ... }.

@@ -117,3 +117,21 @@
}
}
}

.ck[dir="rtl"] .ck.ck-tooltip {
Copy link
Member

@oleq oleq Nov 27, 2020

Choose a reason for hiding this comment

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

This should not be handled in CSS like this. Instead, this should be done at the JS level based on locale.uiLanguageDirection (locale.uiLanguageDirection === 'rtl' ? 'se' : 'sw'). To do that we need a new .ck-tooltip_se style corresponding to the existing .ck-tooltip_sw and documented in the TooltipView#position property docs.


Edit: Not sure if locale.contentLanguageDirection makes more sense than locale.uiLanguageDirection. Take a look at https://ckeditor.com/docs/ckeditor5/latest/features/ui-language.html#setting-the-language-of-the-content before making the decision 😛

@niegowski niegowski requested a review from oleq November 27, 2020 15:12
@oleq oleq merged commit 1ac756a into master Nov 30, 2020
@oleq oleq deleted the i/8335 branch November 30, 2020 14:17
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.

HTML embed editing UI is broken when editor uses an RTL language
3 participants