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

Json viewer #2561

Open
wants to merge 26 commits into
base: release
Choose a base branch
from
Open

Conversation

anjiazhuyouxing
Copy link

中文模板 / Chinese Template

What kind of change does this PR introduce? (check at least one)

  • Bugfix
  • Feature
  • Code style update
  • Refactor
  • Test Case
  • TypeScript definition update
  • Document improve
  • CI/CD improve
  • Branch sync
  • Other, please describe:

PR description

Fixes #

Changelog

🇨🇳 Chinese

  • Feat: 新增json-viewer组件

🇺🇸 English

  • Feat: add json-viewer component

Checklist

  • Test or no need
  • Document or no need
  • Changelog or no need

Other

  • Skip Changelog

Additional information

content/show/jsonviewer/index.md Show resolved Hide resolved
@@ -242,5 +243,6 @@
"stylelint"
]
},
"license": "MIT"
"license": "MIT",
"packageManager": "[email protected]+sha512.a6b2f7906b721bba3d67d4aff083df04dad64c399707841b7acf00f6b133b7ac24255f2652fa22ae3534329dc6180534e98d17432037ff6fd140556e2bb3137e"
Copy link
Collaborator

Choose a reason for hiding this comment

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

这个pacakgeMannger 这行不要带上去

packages/semi-foundation/jsonViewer/foundation.ts Outdated Show resolved Hide resolved
packages/semi-ui/jsonViewer/index.tsx Outdated Show resolved Hide resolved
content/show/jsonviewer/index.md Show resolved Hide resolved
| 属性 | 说明 | 类型 | 默认值 |
|-------------------|------------------------------------------------|---------------------------------|-----------|
| lineHeight | 设置行高 属性 | number | 20 |
| autoHeight | 设置是否自动换行 属性 | boolean | true |
Copy link
Collaborator

Choose a reason for hiding this comment

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

autoHeight 作为是否自动换行的 api 名字是不是有点抽象?

Copy link
Author

Choose a reason for hiding this comment

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

get,改成autoWrap了

| width | 设置宽度 属性 | number | - |
| options | 设置格式化配置 属性 | JsonViewerOptions | - |
| onChange | 设置 value 变化回调 属性 | (value: string) => void | - |
| onValueHover | 设置 value 悬浮回调 属性 | ({value: string, target: HTMLElement}) => HTMLElement | - |
Copy link
Collaborator

Choose a reason for hiding this comment

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

这个是你 storybook 里写的 hoverHandler 吗?

e.preventDefault();
let insertText = '';

if (this._view.options?.formatOptions?.insertSpaces) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

为啥要在 this._view.options?.formatOptions?.insertSpaces 为 true 的时候才能让 Tab 自定义缩进生效?

Copy link
Collaborator

Choose a reason for hiding this comment

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

另外我试了一下,给 formatOptions { tabSize: 8 } 的默认值以后,insertSpaces 的值也不是 true 而是 undefined

Copy link
Author

Choose a reason for hiding this comment

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

tabSize表示缩进的距离,insertSpaces为true表示用空格表示缩进,比如4个空格,为fasle表示用制表符缩进,不过我发现逻辑存在问题,我后续修一下

"lib/*"
],
"dependencies": {
"esbuild": "0.24.0",
Copy link
Collaborator

Choose a reason for hiding this comment

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

esbuild挪到 devDepenencies里去吧,用这个的用户不需要装 esbuild

Copy link

codesandbox-ci bot commented Nov 13, 2024

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit d9fa9e6:

Sandbox Source
pr-story Configuration

| width | 设置宽度 属性 | number | - |
| options | 设置格式化配置 属性 | JsonViewerOptions | - |
| onChange | 设置 value 变化回调 属性 | (value: string) => void | - |
| onValueHover | 设置 value 悬浮回调 属性 | ({value: string, target: HTMLElement}) => HTMLElement | undefined | - |
Copy link
Collaborator

Choose a reason for hiding this comment

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

一般onXX的回调很少会依赖 return 值的。如果这里是 value hover时,通过这个函数 return 了 DOM element,你会插入渲染这个 dom Element,那么叫 renderXXX 更好一些

"peerDependencies": {
"react": ">=16.0.0",
"react-dom": ">=16.0.0"
},
Copy link
Collaborator

Choose a reason for hiding this comment

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

core包需要依赖 react、react-dom吗?

return {
...super.adapter,
getEditorRef: () => this.editorRef.current,
onValueChange: (value) => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

这里统一一下命名吧。semi里,负责调用 props.onXXX的 adapter函数我们统一以 notifyXXX 命名,这样是为了方便做区分。
其他维护者发现 onXXX的行为异常时,只需要去搜 notifyXXX就能很快地找到对应调用链路,同名的话会增加区分难度。

Copy link
Collaborator

Choose a reason for hiding this comment

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

可以参考一下input这里的 adapter函数命名

this.foundation.search(value);
}

changeSearchOptions = (key: string) => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

这里不要出现直接 setState的操作了。所有对 state的更新、对dom的直接操作,都通过 adapter 委托一层。

{this.state.showSearchBar && <div className={`${prefixCls}-search-bar-container`}>
<div className={`${prefixCls}-search-bar`}>
<Input
placeholder="查找"
Copy link
Collaborator

Choose a reason for hiding this comment

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

placeholder 这种有内置的文案“查找”、“替换”,需要做一下 i18n,参考其他组件使用 Locale的做法

<Button icon={<IconClose />} size="small"
theme={'borderless'}
type={'tertiary'}
onClick={() => this.setState({ showSearchBar: false })} />
Copy link
Collaborator

Choose a reason for hiding this comment

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

这个setState也是,收敛回 adapter里面。这样能更快找到所有对 state的读/写操作。

)
);

$semi-json-viewer-search-result-background: rgba(var(--semi-green-2), 1); // JSON search result background 颜色
Copy link
Collaborator

Choose a reason for hiding this comment

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

scss variable不用加 semi-前缀。
css variable加是因为有些用户可能会混用多个 ui lib,有冲突的风险。scss没有这种问题,加上去反而有点繁琐。

* @returns line element
*/
export function getLineElement(node: Node): HTMLElement | null {
return node.parentElement?.closest('.semi-json-viewer-view-line') || null;
Copy link
Collaborator

Choose a reason for hiding this comment

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

这里需要考虑一下 prefixCls被修改的情况,dom不一定还是以 .semi开头

/**
* A position in the editor.
*/
export class Position {
Copy link
Collaborator

Choose a reason for hiding this comment

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

这个有用到吗?

@@ -0,0 +1,4 @@
export interface Token {
Copy link
Collaborator

Choose a reason for hiding this comment

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

这种单个 interface 就没有必要单开一个文档了吧。

/** Based on https://github.com/microsoft/vscode-json-languageservice with modifications for custom requirements */
import { Position } from '../common/position';
import { Range } from '../common/range';
/**
Copy link
Collaborator

Choose a reason for hiding this comment

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

这个文件放在 service 的意义是?

error(message: string): void;
setAsIncomplete(): void;
getNumberOfProposals(): number
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

这两个interface也是只有一个文件在import 使用,单独出来的意义是?
image

Copy link
Collaborator

Choose a reason for hiding this comment

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

因为是reference吗?

}

private _cutHandler() {
console.log('cut');
Copy link
Collaborator

Choose a reason for hiding this comment

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

log移除一下

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.

3 participants