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

IT-5528 | Fixes after upstream update #10

Merged
merged 8 commits into from
Dec 13, 2023
Merged

IT-5528 | Fixes after upstream update #10

merged 8 commits into from
Dec 13, 2023

Conversation

kvas-damian
Copy link

No description provided.

@@ -1,6 +1,6 @@
import InputView from '@ckeditor/ckeditor5-ui/src/input/inputview';
import { View } from 'ckeditor5/src/ui';
Copy link
Author

Choose a reason for hiding this comment

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

FIx import, after changing View to InputView in 2de8f65

InputView automatically emits input event which is handled in

bind.to( () => ( this.value = this.element.getValue( 'latex-expanded' ) ) ),
=> we fetched value from mathlive even when mathlive didn't emit change

@@ -166,7 +184,7 @@ export default class MainFormView extends View {
const t = this.locale.t;

// Create equation input
const mathInput = new LabeledFieldView( this.locale, createLabeledInputText );
const mathInput = new LabeledFieldView( this.locale, createLabeledTextarea );
Copy link
Author

Choose a reason for hiding this comment

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

theme/mathform.css Outdated Show resolved Hide resolved
@@ -36,12 +36,12 @@
"@ckeditor/ckeditor5-theme-lark": "40.1.0",
"@ckeditor/ckeditor5-upload": "40.1.0",
"eslint": "^7.32.0",
"eslint-config-ckeditor5": "^5.1.1",
"eslint-config-ckeditor5": "^5.1.2",
Copy link

Choose a reason for hiding this comment

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

It was a part of isaul32@fffb69b but not 41f075f

Copy link

Choose a reason for hiding this comment

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

Without it, I was getting error about node 20 not being supported.

theme/mathform.css Outdated Show resolved Hide resolved
...this.template.attributes,
style: {
resize: 'both',
overflow: 'auto'
Copy link

Choose a reason for hiding this comment

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

We don't set it now, is that correct? I don't really see the difference with or without it.
@pioziol123 do you remember why you've added it in 2c275e7?

Choose a reason for hiding this comment

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

Hardly, I think it was something with virtual keyboard 🤷🏻‍♂️

Copy link

Choose a reason for hiding this comment

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

Nothing pops out for me after playing with the keyboard. Maybe it's something visible only in the app? @kvas-damian how about adding it for safety? Or do you prefer to put more time into searching what is it for?

Copy link
Author

Choose a reason for hiding this comment

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

ok, will add it for safety. I've seen enough strange things with styles in this module, so better safe than sorry ;)

Copy link

@rogatty rogatty left a comment

Choose a reason for hiding this comment

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

The logic is fine and it works as expected. I just have two doubts about styles, but it's not blocking.

@kvas-damian kvas-damian merged commit 19ed968 into master Dec 13, 2023
@kvas-damian kvas-damian deleted the IT-5528-fixes branch December 13, 2023 10:59
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.

3 participants