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

#382 Read only text mode. #383

Merged
merged 4 commits into from
Apr 12, 2017
Merged

#382 Read only text mode. #383

merged 4 commits into from
Apr 12, 2017

Conversation

walkerrandolphsmith
Copy link
Contributor

Addresses #382. I have also added an example file in addition to the changes in ./src. I didn't want to make the surface area of options any larger, so I decided to use onEditable to determine if the textarea in text mode is editable. If the onEditable key is absent in the options than the content is editable. If the key is present and has a value that is a function, it will determine whether the text area is editable based on the result of onEditable's invocation.

If hijacking the onEditable option is not appropriate I'd love to chat about the desired api of options.

@josdejong
Copy link
Owner

Thanks Walker, looks good!

Two remarks:

  • It would be nice if we could make the editor readonly when in mode code too
  • Thanks for adding an example too. Can you rename it to 09_readonly_text_mode.html?

@josdejong
Copy link
Owner

If hijacking the onEditable option is not appropriate I'd love to chat about the desired api of options.

I agree with you, using onEditable for making the text mode readonly feels a bit like misusing the existing option. On the other hand it keeps the API compact and consistent. I think your solution is elegant.

@walkerrandolphsmith
Copy link
Contributor Author

walkerrandolphsmith commented Apr 11, 2017

Okay, I can take a look into supporting read only code modes. I can rename the example file.

@walkerrandolphsmith
Copy link
Contributor Author

So that should allow the code mode to be configured to be read only as well.

There was a moment where I could't decide which styling was most appropriate:

aceEditor.setOptions({ readOnly: isReadOnly });
aceEditor.setOptions({readOnly: isReadOnly});
aceEditor.setOptions({
    readOnly: isReadOnly
});

Would you be interested in adding lint rules and eslint to enforce style guides outlined in the contributing doc? I can spin off a new feature and associated PR.

@walkerrandolphsmith
Copy link
Contributor Author

Rebased the upstream develop branch.

@josdejong
Copy link
Owner

Thanks, looks good!

Reminder to myself: add a sentence about that you can use onEditable for modes text and code too in the docs.

@josdejong josdejong merged commit eadf5c6 into josdejong:develop Apr 12, 2017
@josdejong
Copy link
Owner

As for the styling: I tend to use the styling you used in the PR too lately. A linter would be welcome! Would be nice if you could set that up that for jsoneditor, thanks beforehand.

@josdejong josdejong mentioned this pull request Apr 15, 2017
@walkerrandolphsmith walkerrandolphsmith deleted the develop branch April 23, 2017 03:04
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.

2 participants