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

Code editor settings #129

Merged
merged 51 commits into from
May 19, 2016
Merged

Code editor settings #129

merged 51 commits into from
May 19, 2016

Conversation

ortichon
Copy link
Member

@ortichon ortichon commented May 18, 2016

Change Summary

  • Integrate ace.js code editor settings in Kibibit's settings [resolves integrate ace.js settings with kibibit #118]

    Added setters and getters to have a two-way binding between ace.js settings and Kibibit's settingsService

More info

settings added:

  1. ruler
  2. tab width
  3. font-size
  4. show/hide gutter
  5. enable/disable line wrap
  6. enable/disable read only mode
  7. enable/disable soft tabs mode
  8. syntax mode
  9. themes

Before you submit a PR, make sure you did the following things:

  • did you link this PR to an issue?

Make sure there's an issue open about the change you did (open one if there isn't). link this PR to that issue by writing resolves #{{issue_number}}. If this PR had several mission, link each issue in its parallel mission.

  • did you lint your changes to both javascript and scss?

hound can be pretty rough at keeping our code clean and readable! make sure you format your code by running either gulp format or gulp lint and cleaning out all the _lint errors. If you won't, you'll have a _long conversation with hound :-)

  • "I'm pretty sure I'll be able to read and understand this PR, even if I wasn't the author." - _said the PR author_

This change is Reviewable

@thatkookooguy
Copy link
Member

thatkookooguy commented May 19, 2016

@thatkookooguy Oh darn, I forgot about the highlight as well. reject my LGTM
We should add that. Use $primary

Approved with PullApprove

@thatkookooguy
Copy link
Member

thatkookooguy commented May 19, 2016

@thatkookooguy cancel my LGTM

Rejected with PullApprove

@thatkookooguy thatkookooguy temporarily deployed to kibibit-demo-pr-129 May 19, 2016 07:36 Inactive
@thatkookooguy thatkookooguy temporarily deployed to kibibit-demo-pr-129 May 19, 2016 09:15 Inactive

&:focus {
color: $primary;
background: rgb(238,238,238);

Choose a reason for hiding this comment

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

Commas in function arguments should be followed by a single space
Color literals like rgb(238, 238, 238) should only be used in variable declarations; they should be referred to via variable everywhere else.

// this is important because we want to apply the style
// to the selected item (which is excluded in the previous styling rule)
&:focus:not([disabled]) {
color: $primary;

Choose a reason for hiding this comment

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

Properties should be ordered background, color

@thatkookooguy
Copy link
Member

Added a hound exception for now for number of attribute inside a single class.

I need to disable all those checks globally already, it's just the last time I tried to set my own rules for hound, it didn't work :-)

@thatkookooguy
Copy link
Member

thatkookooguy commented May 19, 2016

now I'm waiting for a code review from you on this commit ( @ortichon )

if you think this is good, I'll give you my LGTM after that

Approved with PullApprove

@thatkookooguy
Copy link
Member

thatkookooguy commented May 19, 2016

cancel my LGTM

Rejected with PullApprove

@thatkookooguy thatkookooguy mentioned this pull request May 19, 2016
@thatkookooguy
Copy link
Member

thatkookooguy commented May 19, 2016

@thatkookooguy LGTM

Approved with PullApprove

@ortichon
Copy link
Member Author

@thatkookooguy Test - telegram comment

@ortichon
Copy link
Member Author

@thatkookooguy another test

@thatkookooguy
Copy link
Member

thatkookooguy commented May 19, 2016

LGTM

Previously, ortichon (Or Tichon) wrote…

@thatkookooguy another test


Comments from Reviewable

Approved with PullApprove

@thatkookooguy thatkookooguy merged commit 3c95839 into master May 19, 2016
@thatkookooguy thatkookooguy deleted the code-editor-settings branch May 19, 2016 13:05
neilkalman-redkix pushed a commit that referenced this pull request Jul 4, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

integrate ace.js settings with kibibit
4 participants