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

CKEditor Backward Compatibility #224

Merged
merged 9 commits into from
Nov 24, 2020
Merged

Conversation

tobiasschweizer
Copy link
Contributor

resolves DSP-788

Reapplies changes from #212 which was merged by accident.

@tobiasschweizer tobiasschweizer added bug Something isn't working user requirement labels Nov 5, 2020
@tobiasschweizer tobiasschweizer self-assigned this Nov 5, 2020
@tobiasschweizer
Copy link
Contributor Author

@loicjaouen Sorry, there was some merge accident. This PR replaces #212.

Copy link
Collaborator

@loicjaouen loicjaouen left a comment

Choose a reason for hiding this comment

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

this all looks fine to me

@loicjaouen
Copy link
Collaborator

I am not sure that is solves DSP-788 though

let's merge and consider DSP-788 seperately

@tobiasschweizer
Copy link
Contributor Author

I am not sure that is solves DSP-788 though

let's merge and consider DSP-788 seperately

This PR is about DSP-788 :-) What's missing?

@loicjaouen
Copy link
Collaborator

This PR is about DSP-788 :-) What's missing?

sorry @tobiasschweizer , I didn't see your comment here but I answered it at length in DSP-788

@tobiasschweizer
Copy link
Contributor Author

This PR is about DSP-788 :-) What's missing?

sorry @tobiasschweizer , I didn't see your comment here but I answered it at length in DSP-788

I saw this (rather impressive testing) :-) So we can merge this PR but leave DSP-788 open?

@@ -6,6 +6,10 @@
font-family: monospace;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@kilchenmann I have just notice that these styles are overridden by the app's main.scss:

* {
  font-family: Arial, Helvetica, sans-serif;
}

Any idea what could be done about that?

Copy link
Contributor

Choose a reason for hiding this comment

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

Hm, we shouldn't use * { in this css. But it can happen in all apps.

Copy link
Contributor

@kilchenmann kilchenmann Nov 24, 2020

Choose a reason for hiding this comment

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

ng-deep is anyway deprecated since Angular v6. Could you add the two setups in ng-deep to the viewer.scss?

.ck-content code {
  font-family: monospace !important;
}
.ck-content pre {
   background-color: inherit !important;
 }

But not sure if this helps...

Copy link
Contributor

Choose a reason for hiding this comment

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

@tobiasschweizer I just updated the code in the previous comment

@kilchenmann kilchenmann self-requested a review November 24, 2020 16:13
@tobiasschweizer
Copy link
Contributor Author

@kilchenmann Mörci!

@tobiasschweizer tobiasschweizer merged commit ec2ae59 into main Nov 24, 2020
@tobiasschweizer tobiasschweizer deleted the wip/dsp-788-ckeditor-comp2 branch November 24, 2020 16:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working user requirement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants