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

Standardise code style and remove eslint disables #1180

Merged
merged 1 commit into from
Apr 10, 2020

Conversation

ang-zeyu
Copy link
Contributor

@ang-zeyu ang-zeyu commented Apr 4, 2020

What is the purpose of this pull request? (put "X" next to an item, remove the rest)

• [x] Other, please explain:

What is the rationale for this request?

What changes did you make? (Give an overview)

Example of style regression fixed

$('modal')	     
  .remove();	     
$('panel')	
  .remove();

// to
$('modal').remove();
$('panel').remove();

Is there anything you'd like reviewers to focus on?
na

Testing instructions:
na

Proposed commit message: (wrap lines at 72 characters)
Standardise code style and remove eslint disables

@openorclose
Copy link
Contributor

I don't think we should need to go in and manually fix styling? If it's accepted by eslint, that should be good enough.

Too much time will be spent on commenting about code style in PRs otherwise.

That being said, it's good that some of the unnecessary eslint disables are removed.

@ang-zeyu
Copy link
Contributor Author

ang-zeyu commented Apr 4, 2020

I don't think we should need to go in and manually fix styling? If it's accepted by eslint, that should be good enough.

Too much time will be spent on commenting about code style in PRs otherwise.

That being said, it's good that some of the unnecessary eslint disables are removed.

Most of the style regressions were rather awkward, you could refer here #979 (review)
I think its always good to keep the code more readable especially for future developers

Too much time will be spent on commenting about code style in PRs otherwise.

Yes, this PR is specifically made for reverting the unintended code style changes though 😁

Copy link
Member

@yamgent yamgent left a comment

Choose a reason for hiding this comment

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

Thanks, the cleanup looks pretty good 👍

Copy link
Contributor

@nbriannl nbriannl left a comment

Choose a reason for hiding this comment

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

Could you rebase your PR 🙂

Copy link
Contributor

@nbriannl nbriannl left a comment

Choose a reason for hiding this comment

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

👌

@ang-zeyu ang-zeyu merged commit da8c0f5 into MarkBind:master Apr 10, 2020
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.

4 participants