-
Notifications
You must be signed in to change notification settings - Fork 5
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
various cleanup chores in the editor #136
Conversation
nvdk
commented
Jul 23, 2021
•
edited
Loading
edited
- convert ContentEditable component to glimmer
- convert ContentEditable component to typescript
- upgrade ember-concurrency
- convert RdfaEditor component to glimmer
- convert RdfaEditor component to typescript
somewhat coincidentally this also fixes https://binnenland.atlassian.net/browse/GN-2318 |
*/ | ||
@tracked editor; | ||
@tracked editor?: PernetRawEditor; |
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.
since this can now be undefined, there need to be some extra undefined checks when editor is accessed
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.
one option is a getter that throws when editor is null (nothing should try to run before editor is initialized, this may also alert us to some initialization bugs we were having)
if that generates too many problems, simply asserting this is non-null (@tracked editor!: PernetRawEditor
) is also an option, though we should avoid that if possible
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.
such a getter should also be added in rdfa-editor.ts
, since we have the same problem here as in content-editable.ts
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.
that won't work there though, since the editor is used in the template before init. all usages do check if editor is null though so less use to do so there
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.
Love this, typescript ahoi!
*/ | ||
@tracked editor; | ||
@tracked editor?: PernetRawEditor; |
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.
one option is a getter that throws when editor is null (nothing should try to run before editor is initialized, this may also alert us to some initialization bugs we were having)
if that generates too many problems, simply asserting this is non-null (@tracked editor!: PernetRawEditor
) is also an option, though we should avoid that if possible
addon/components/rdfa/rdfa-editor.ts
Outdated
if (this.profile != this.eventProcessor.profile) { | ||
@action | ||
updateProfile() { | ||
if (this.eventProcessor && this.profile != this.eventProcessor.profile) { |
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.
might be worth going through and making sure we use triple equals everywhere (kinda surprised and slightly alarmed this doesn't trigger the linter)
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.
that would be a lot of linter errors:
nielsv@smallbox ~/Code/say-editor/ember-rdfa-editor $ grep -r ' == ' addon/ | wc -l
164
nielsv@smallbox ~/Code/say-editor/ember-rdfa-editor $ grep -r ' != ' addon/ | wc -l
27
@nvdk WARNING: base branch master |
having two files both named raw-editor.ts was just confusing
*/ | ||
@tracked editor; | ||
@tracked editor?: PernetRawEditor; |
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.
such a getter should also be added in rdfa-editor.ts
, since we have the same problem here as in content-editable.ts
@abeforgit shall I merge? |