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

Set readOnly state for prod dynmic linux. Closes #2565 #2579

Merged
merged 1 commit into from
Apr 18, 2018
Merged

Conversation

ahmelsayed
Copy link
Contributor

No description provided.

@@ -69,14 +70,28 @@ export class MonacoEditorDirective {

@Input('disabled')
set disabled(value: boolean) {
if (value !== this._disabled) {
if (typeof value === 'boolean' && value !== this._disabled) {
Copy link
Contributor

Choose a reason for hiding this comment

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

why add a check for value's type? also if it is not boolean should we log an error?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I only added the check because I saw it being null. It's set from the view using readOnlyObservable$ | async I'm guessing that's trying to access readOnlyObeservable before it's set, which the | async pipe converts to null.

We can fix it so that this isn't called until the read only observable not be undefined, or just check that the type is not null here, which typeof value === 'boolean' is indirectly doing. But maybe value !== null will be clearer here. What do you think?

Copy link
Contributor

Choose a reason for hiding this comment

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

I would prefer an explicit check for !== null. I also wonder if we should make the signature nullable? (value?: boolean).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good point. I'll do that. Change sig to nullable and update the check for null

}

const setEditorReadOnly = () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

why follow this pattern of creating a setEditorReadOnly function?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

because of the comment above :)

// the editor loads asynchronously outside of our control
// if disabled has been set to true, try to set in a timeout.
// Otherwise we can miss the first "readOnly" set.
// clear the timeout if there was already one.

Copy link
Contributor

Choose a reason for hiding this comment

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

Makes sense :)

Copy link
Contributor

@nertim nertim left a comment

Choose a reason for hiding this comment

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

just some basic questions.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants