-
Notifications
You must be signed in to change notification settings - Fork 3
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
UHF-8387 CKEditor5 styles #724
Conversation
…, small overrides for the CKEditor toolbar styles
…le declarations on ckeditor.scss file so that the possiblity of conflicting styles is minimal in the future
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Works nicely 👍 added a small comment on slack. Check that before merging.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, but there are some things still to consider.
src/scss/ckeditor.scss
Outdated
text-decoration: underline; | ||
|
||
// External link styles | ||
&[href^='https://']::after { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about http://
links?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, having https://
in front of a link is not enough to detect it as an external link. This approach would mark links with https://www.hel.fi/fi as external.
We should run the actual script that checks if the link is external or not. Or if we really need to do it with css, we should check for our common urls like:
&:is([href^='http://'],[href^='https://']):not([href^='http://hel.fi/'],[href^='https://hel.fi/'],[href^='http://www.hel.fi/'],[href^='https://www.hel.fi/'])::after {
But I would recommend against it as then external teams using our HDBT code would be tied to these hardcoded values.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah this is tricky - maybe we can add a script to run with the ckeditor and modify its dom but it would get this issue quite heavily out of estimate so I think its best if we just leave the external link styles away. This is an improvement already as it is compared to the current ckeditor interface.
src/scss/ckeditor.scss
Outdated
color: $color-black-90; | ||
font-family: $base-font-family; | ||
|
||
> div { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this targeting > div
and not > *
?
Now paragraphs can be wider than they're allowed to be in content.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Its targeting the div directly under the ck-content and not the actual content yet. The div is a wrapping div for all the user edited content that will be written there. So the paragraphs can't be wider in this case. I will add a comment that explains this better.
…om ckeditor styles, added stylelint-disable for the ckeditor styles
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Otherwise great job, but the > *
should be added due to the differences between our environments. The star selector is more "safe". Approved conditionally.
…t works on all cases even if there isn't div selector wrapping all the content on ckeditor
UHF-8387
What was done
How to install
git pull origin dev
make fresh
composer require drupal/helfi_platform_config:dev-ckeditor5
composer require drupal/hdbt:dev-UHF-8387_ckeditor5_styles
make drush-updb drush-cr
make shell
and in shell:drush locale:check; drush locale:update; drush cr
How to test
Designers review