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.

Sign up for GitHub

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

Jump to bottom

[Frontend] Prettier config to be consistent with existing code style #2409

Merged
merged 3 commits into from
Oct 17, 2019
Merged

[Frontend] Prettier config to be consistent with existing code style #2409

merged 3 commits into from
Oct 17, 2019

Conversation

Bobgy
Copy link
Contributor

@Bobgy Bobgy commented Oct 16, 2019 •
edited
Loading

Config is proposed here: #2406 (comment)

/area front-end

This change is Reviewable

@Bobgy Bobgy mentioned this pull request Oct 16, 2019
Closed
@Bobgy Bobgy changed the title Install and configure prettier to be consistent with existing code style [Frontend] Prettier config to be consistent with existing code style Oct 16, 2019
@Bobgy
Copy link
Contributor Author

Bobgy commented Oct 17, 2019

/assign @jingzhang36

rmgogogo
rmgogogo reviewed Oct 17, 2019
View reviewed changes
frontend/README.md

To understand more what prettier is: [What is Prettier](https://prettier.io/docs/en/index.html).

### IDE Integration
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure whether it has conflict with VSCode setting for Pantheon.

If yes, configure two IDEs may not be good. In such case, we may revisit this part later. Maybe just reuse Pantheon setting?

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 think there are a few problems with pantheon setting (clang-format):

  1. It is very different from existing code base
  2. It has no support for tsx (react)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If yes, configure two IDEs may not be good. In such case, we may revisit this part later. Maybe just reuse Pantheon setting?

We can co-maintain a vscode workspace setting just for this repo.

rmgogogo
rmgogogo reviewed Oct 17, 2019
View reviewed changes
Copy link
Contributor

@rmgogogo rmgogogo left a comment

Choose a reason for hiding this comment

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

/lgtm

@Bobgy
Copy link
Contributor Author

Bobgy commented Oct 17, 2019

/approve

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Bobgy

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot merged commit 8bdc4ba into kubeflow:master Oct 17, 2019
@Bobgy Bobgy deleted the fe_prettier branch October 17, 2019 06:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rmgogogo rmgogogo rmgogogo left review comments

@IronPan IronPan Awaiting requested review from IronPan

@neuromage neuromage Awaiting requested review from neuromage

Assignees

@rmgogogo rmgogogo

@jingzhang36 jingzhang36

Labels
approved area/engprod area/frontend lgtm size/XXL
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@Bobgy @k8s-ci-robot @rmgogogo @jingzhang36