Skip to content

[Zeppelin-1213] Customize editor configuration - #1218

Closed
cloverhearts wants to merge 4 commits into
apache:masterfrom
cloverhearts:ZEPPELIN-1213
Closed

[Zeppelin-1213] Customize editor configuration#1218
cloverhearts wants to merge 4 commits into
apache:masterfrom
cloverhearts:ZEPPELIN-1213

Conversation

@cloverhearts

@cloverheartscloverhearts commented Jul 22, 2016

Copy link
Copy Markdown
Member

What is this PR for?

Customize editor configuration

  • Each user can have different editor setting(For example, editor theme, indentation, font size, editor print margin, etc)
  • Each user can have different editor setting for each interpreter. (E.g %spark -> font size, indent, theme)

What type of PR is it?

Feature

Todos

  • create editor configuration setting page.
  • configure theme
  • configure support language
  • change font size and indent theme
  • editor config serialize / deserialize
  • preview window.
  • create and delete, and save.
  • add web socket event for code editor setting.
  • apply setting for each interpreter tag
  • if show line number setting already exists in paragraph, paragraph setting has priority over editor setting

What is the Jira issue?

https://issues.apache.org/jira/browse/ZEPPELIN-1213

How should this be tested?

  1. click to user name menu in navbar and click to Editor Configuration menu.
  2. set up editor setting. (and using preview pargraph)
  3. click to save button.
  4. move to any paragraph.
  5. check to apply setting.
  6. check to exists conf/code-editor.json in zeppelin directory.

Screenshots (if appropriate)

setup

2222

Questions:

  • Does the licenses files need update? no
  • Is there breaking changes for older versions? no
  • Does this needs documentation? no

@cloverheartscloverhearts changed the title [Zeppelin-1213] Code Editor Configuration Personalization[Zeppelin-1213] Customize editor configurationJul 22, 2016
@astroshim

Copy link
Copy Markdown
Contributor

@cloverhearts Really great feature! I can't wait to build and run this.

@raulgomis

Copy link
Copy Markdown

+1

@corneadoug

Copy link
Copy Markdown
Contributor

This PR is way too big to be reviewed properly.
I'm not sure every settings are necessary (especially from the start).
And I also have issues with the fact that every Theme from Ace is included.

@cloverhearts

Copy link
Copy Markdown
MemberAuthor

Thank you for feedback @corneadoug .
I can possible divide to this pr.
and I have a question.

I also have issues with the fact that every Theme from Ace is included.

Can you give me an explanation why that is a problem?
If you have a problem, I need to clear a part of the theme changes.
a very easy fix.
I thought that people would want to coding in a theme familiar to them.

Once again thank you for your message.

@corneadoug

Copy link
Copy Markdown
Contributor

@cloverhearts it would be nice to divide it in multiple PRs and commits.
For the theme, I'm not against them wanting to change it, however in this case crowding the build by importing all the Ace theme inside the build, even though 70% of them will probably not be used.

@cloverhearts

Copy link
Copy Markdown
MemberAuthor

I removed the feature to change the theme.
It will also split into a new PR.

@astroshimastroshim mentioned this pull request Jul 26, 2016
1 task
"src-noconflict/mode-markdown.js",
"src-noconflict/mode-sh.js",
"src-noconflict/mode-r.js",
"src-noconflict/mode-scala.js",

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The indention does not look aligned with the parent "main" above. Previous one was.

Also, adding such changes to PR that implements new functions makes code review task harder (and slower) - now everyone who reads this need to think, if this changes are part of the new feature or not.

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

@bzz
I will fix.

@bzz

bzz commented Jul 28, 2016

Copy link
Copy Markdown
Member

I agree with @corneadoug that this is a big change to review.

One more thing - @cloverhearts can you please add some tests for this feature?
See AuthenticationIT.java for an example of Integration Test

@corneadoug

Copy link
Copy Markdown
Contributor

@cloverhearts Don't hesitate to ping me when you start making the small PRs

@cloverhearts

Copy link
Copy Markdown
MemberAuthor

@bzz@corneadoug
Okay, Thank you very much

@bzz

bzz commented Aug 3, 2016

Copy link
Copy Markdown
Member

Shall we close this one now?

@bzz

bzz commented Aug 11, 2016

Copy link
Copy Markdown
Member

@cloverhearts ping

@cloverhearts

Copy link
Copy Markdown
MemberAuthor

@bzz i am sorry to late for response.
The PR was divided.
#1256
and jira issue subtask
https://issues.apache.org/jira/browse/ZEPPELIN-1213

@bzz

bzz commented Aug 11, 2016

Copy link
Copy Markdown
Member

That looks great! Thank you @cloverhearts let's close this PR then.

@cloverhearts

Copy link
Copy Markdown
MemberAuthor

@bzz Okay!

@hyonaldo

Copy link
Copy Markdown

How can I use this feature?
Though I've installed latest build version, I could not find "Editor Configuration menu" anywhere.

@cloverhearts

Copy link
Copy Markdown
MemberAuthor

@hyonaldo
Sorry,
We can not support this feature on zeppelin.
I do not care for this feature into master.
It also differs from the current source in many ways.
Re-implementation is required.
I have no plans to reimplement this until now.
Perhaps, if you want this feature.
You can create an issue in our jira. (https://issues.apache.org/jira/browse/ZEPPELIN)
Sorry, again.

Sign up for freeto 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.

6 participants

@cloverhearts@astroshim@raulgomis@corneadoug@bzz@hyonaldo