backend.ai-webui icon indicating copy to clipboard operation
backend.ai-webui copied to clipboard

Let's provide code convention guide and apply rules to config files.

Open lizable opened this issue 2 years ago • 15 comments

Is your feature request related to a problem? Please describe. A clear and concise description of what the problem is. Ex. I'm always frustrated when [...] 기능 요청이 특정 문제에 연관된 것이라면 여기에 그 문제를 설명해주세요. It's time to provide a newbie-friendly guide. Code convention guide would be a good example. There are lots of code conventions that are only exposed in code reviewing. There are two benefits of defining code conventions:

  • Reduce miscellaneous checks and make reviewers can concentrate on the code itself more easily.
  • Those who want to contribute to the project can get insights from code conventions, which results in more and more contributions.

Describe the solution you'd like A clear and concise description of what you want to happen. 어떤 기능이 있으면 좋겠는지 자세히 설명해주세요.

  • Create a code convention rule in the markdown file. If there's a good example, then add reference for it. (e.g. Google TypeScript Style Guide)
  • Update typescript config file to notice when code violates the rules.
  • Add project-specific notes if it's needed. (e.g. Circumventing both TypeError and ImportError on using classes defined in the project)

Describe alternatives you've considered A clear and concise description of any alternative solutions or features you've considered. 혹시 다른 대안들을 생각해본 적이 있다면 함께 적어주세요. We may just use "JS standard style", if all of the works seem in vain...

Additional context Add any other context or screenshots about the feature request here. 기능 요청에 대해 보다 잘 이해할 수 있는 다른 맥락을 기술해주세요.

lizable avatar Jul 19 '22 04:07 lizable

Also, there will be code refactoring chores after the code convention is defined. Such as unifying using Camel case or Kebab case, etc.

lizable avatar Jul 19 '22 04:07 lizable

I found a PR #1306 that updates lint rules. How's it going?

Jaewoook avatar Jul 22 '22 08:07 Jaewoook

I found a PR #1306 that updates lint rules. How's it going?

We need to dig and polish (manually, if possible) rules, since it's not perfectly fit into our expectations.

lizable avatar Jul 22 '22 08:07 lizable

I would like to suggest some conventions.

Suggestion: Use consistent variable name

Although the suggestion is ambiguous, I'm going to handle some case when naming variable.

1. element reference variable

In page components, there are so many variable referencing html element, so developer cannot determine what element variable references what element. For example, let's suppose there is a variable named someCount, the someCount variable can be select and input element reference. Then how about someCountSelect or someCountInput? Adding element type suffix is more clear to determine what element is.

2. element naming convention

There are many way to naming variable like kebab-case, camelCase, snake_case, PascalCase, etc. We can choose naming convention to naming variable and HTML classes, ids, and more. My suggestion is using camelCase in class property / method, kebab-case in html class / id to naming it. I think there will be some edge case like vfolder. In this case, how about capitalize the shorten letter and also next letter, like VFolder?

Any feedback, opinion are welcome 😃

Jaewoook avatar Jul 25 '22 14:07 Jaewoook

Suggestion: import order

I suggest applying the import/order rule. You can read more detail in following link.

rule documentation: https://github.com/import-js/eslint-plugin-import/blob/main/docs/rules/order.md

Jaewoook avatar Jul 25 '22 14:07 Jaewoook

Suggestion: lit-a11y rules

The lit-a11y (accessibility) rules already applied in this project but there's so many lit-a11y errors in this project. Good accessibility makes better web experience for user. Let's follow the lit-a11y rules strictly.

Jaewoook avatar Jul 25 '22 14:07 Jaewoook

Suggestion: component method order (component structure)

I think component can be separated as 4 part - variables / event handlers / renders / operation logics

Each component has different order of this parts. Consistent order makes better readability for developer to find what they want to see.

I suggest this component structure:

  • class Component
    • variable declaration
      • with initializer
    • operation logics
    • event handler
    • render
      • sub-rrender
      • main render

Jaewoook avatar Jul 25 '22 15:07 Jaewoook

I would like to suggest some conventions.

Suggestion: Use consistent variable name

Although the suggestion is ambiguous, I'm going to handle some case when naming variable.

1. naming element reference variable

In page components, there are so many variable referencing html element, so developer cannot determine what element variable references what element. For example, let's suppose there is a variable named someCount, the someCount variable can be select and input element reference. Then how about someCountSelect or someCountInput? Adding element type suffix is more clear to determine what element is.

2. element naming convention

There are many way to naming variable like kebab-case, camelCase, snake_case, PascalCase, etc. We can choose naming convention to naming variable and HTML classes, ids, and more. My suggestion is using camelCase in class property / method, kebab-case in html class / id to naming it. I think there will be some edge case like vfolder. In this case, how about capitalize the shorten letter and also next letter, like VFolder?

Any feedback, opinion are welcome 😃

I agree. I've tentatively applied the first suggestion on here(#1385). One more addition to the first suggestion, I would also like to suggest adding a ~List suffix when the member variable type is Array<T>. And for element naming conventions, let's sum up the edge cases (like VFolder) somewhere so that we can find the patterns and apply the alternative rules.

lizable avatar Jul 25 '22 15:07 lizable

Suggestion: component method order (component structure)

I think component can be separated as 4 part - variables / event handlers / renders / operation logics

Each component has different order of this parts. Consistent order makes better readability for developer to find what they want to see.

I suggest this component structure:

  • class Component
    • variable declaration
      • with initializer
    • operation logics
    • event handler
    • render
      • sub-rrender
      • main render

Other component structure suggestion:


Event handler after render function

  • class Component
    • variable declaration
      • with initializer
    • operation logics
    • render
      • main render
      • sub-rrender
    • event handler

variable initializer and operation logics after render and handlers

  • class Component
    • variable declaration
    • render
      • main render
      • sub-rrender
    • event handler
    • variable initializer
    • operation logics

Jaewoook avatar Jul 25 '22 15:07 Jaewoook

Suggestion: component method order (component structure)

I think component can be separated as 4 part - variables / event handlers / renders / operation logics

Each component has different order of this parts. Consistent order makes better readability for developer to find what they want to see.

I suggest this component structure:

  • class Component

    • variable declaration

      • with initializer
    • operation logics

    • event handler

    • render

      • sub-render
      • main render

Good suggestion. In addition to your opinion, I suggest placing style codes right before the main render codes. In some files, the style code is after the constructor, and in some files before the main render. I want to unify this, too.

agatha197 avatar Jul 25 '22 15:07 agatha197

Suggestion: component method order (component structure)

I think component can be separated as 4 part - variables / event handlers / renders / operation logics

Each component has different order of this parts. Consistent order makes better readability for developer to find what they want to see.

I suggest this component structure:

  • class Component

    • variable declaration

      • with initializer
    • operation logics

    • event handler

    • render

      • sub-rrender
      • main render

We can use this rule to achieve our goals

documentation link: https://typescript-eslint.io/rules/member-ordering/

Jaewoook avatar Jul 25 '22 15:07 Jaewoook

Suggestion: component method order (component structure)

I think component can be separated as 4 part - variables / event handlers / renders / operation logics Each component has different order of this parts. Consistent order makes better readability for developer to find what they want to see.

I suggest this component structure:

  • class Component

    • variable declaration

      • with initializer
    • operation logics

    • event handler

    • render

      • sub-render
      • main render

Good suggestion. In addition to your opinion, I suggest placing style codes right before the main render codes. In some files, the style code is after the constructor, and in some files before the main render. I want to unify this, too.

How about split style code to component's outside? style code annoys me when I read the javascript source.

I think there are two methods we could try:

  • class
    • class sources
  • styles

or separate files like this

  • components/
    • some-page/
      • index
      • some-page-component
      • some-page-style

Jaewoook avatar Jul 25 '22 15:07 Jaewoook

Suggestion: component method order (component structure)

I think component can be separated as 4 part - variables / event handlers / renders / operation logics Each component has different order of this parts. Consistent order makes better readability for developer to find what they want to see.

I suggest this component structure:

  • class Component

    • variable declaration

      • with initializer
    • operation logics

    • event handler

    • render

      • sub-render
      • main render

Good suggestion. In addition to your opinion, I suggest placing style codes right before the main render codes. In some files, the style code is after the constructor, and in some files before the main render. I want to unify this, too.

How about split style code to component's outside? style code annoys me when I read the javascript source.

I think there are two methods we could try:

  • class

    • class sources
  • styles

or separate files like this

  • components/

    • some-page/

      • index
      • some-page-component
      • some-page-style

https://lit.dev/docs/components/styles/#external-stylesheet Perhaps this may help to figure out the best way to handle styles

lizable avatar Jul 25 '22 15:07 lizable

Suggestion: component method order (component structure)

I think component can be separated as 4 part - variables / event handlers / renders / operation logics Each component has different order of this parts. Consistent order makes better readability for developer to find what they want to see.

I suggest this component structure:

  • class Component

    • variable declaration

      • with initializer
    • operation logics

    • event handler

    • render

      • sub-render
      • main render

Good suggestion. In addition to your opinion, I suggest placing style codes right before the main render codes. In some files, the style code is after the constructor, and in some files before the main render. I want to unify this, too.

How about split style code to component's outside? style code annoys me when I read the javascript source. I think there are two methods we could try:

  • class

    • class sources
  • styles

or separate files like this

  • components/

    • some-page/

      • index
      • some-page-component
      • some-page-style

https://lit.dev/docs/components/styles/#external-stylesheet Perhaps this may help to figure out the best way to handle styles

'link styles externally' dost not mean using tag. We can just import styles and set it to class's static property like this.

SomeStyles.ts

import {css} from 'lit';

export const outsideStyles = [
    css`/** ...styles go here */`;
];

SomeComponent.ts

import {outsideStyles} from './styles-from-outside';

class SomeComponent extends LitElement {
    static get styles() {
        return outsideStyles;
    }
}

Jaewoook avatar Jul 25 '22 16:07 Jaewoook

Suggestion: component method order (component structure)

I think component can be separated as 4 part - variables / event handlers / renders / operation logics

Each component has different order of this parts. Consistent order makes better readability for developer to find what they want to see.

I suggest this component structure:

  • class Component
  • variable declaration
* with initializer
  • operation logics
  • event handler
  • render
* sub-render
* main render

Good suggestion. In addition to your opinion, I suggest placing style codes right before the main render codes. In some files, the style code is after the constructor, and in some files before the main render. I want to unify this, too.

How about split style code to component's outside? style code annoys me when I read the javascript source.

I think there are two methods we could try:

  • class
  • class sources
  • styles

or separate files like this

  • components/
  • some-page/
* index
* some-page-component
* some-page-style

https://lit.dev/docs/components/styles/#external-stylesheet Perhaps this may help to figure out the best way to handle styles

'link styles externally' dost not mean using tag. We can just import styles and set it to class's static property like this.

SomeStyles.ts


import {css} from 'lit';



export const outsideStyles = [

    css`/** ...styles go here */`;

];

SomeComponent.ts


import {outsideStyles} from './styles-from-outside';



class SomeComponent extends LitElement {

    static get styles() {

        return outsideStyles;

    }

}

Okay, I get it. We've already applied this kind of code splitting in backend-ai-general-styles component (and other components ends with ~styles), so refactoring current compnents especially on styles definition could be as simple as that. (though we have more than 50 components to split, 🥲)

lizable avatar Jul 25 '22 16:07 lizable