feat: Add code-view source code #3
Conversation
rubencarvalho
changed the title
| import Popover from "@cloudscape-design/components/popover"; | ||
| import StatusIndicator from "@cloudscape-design/components/status-indicator"; | ||
| import { useState } from "react"; | ||
|
|
There was a problem hiding this comment.
This can be deleted when an official component exists.
There was a problem hiding this comment.
We cannot un-publish public APIs! Let's talk about this.
There was a problem hiding this comment.
This is an internal, unofficial API but I think it's best if we just get rid of this entirely.
package.json
Outdated
| @@ -0,0 +1,108 @@ | |||
| { | |||
| "name": "@cloudscape-design/code-view", | |||
| "type": "module", | |||
There was a problem hiding this comment.
Is this safe? I don't think we've done this for any other public NPM package so far.
| "./highlight/xml": "./dist/highlight/xml.js", | ||
| "./highlight/yaml": "./dist/highlight/yaml.js" | ||
| }, | ||
| "imports": {}, |
There was a problem hiding this comment.
Removed on new PR
| "dependencies": { | ||
| "ace-code": "^1.32.0", | ||
| "clsx": "^1.2.1", | ||
| "rollup-plugin-ignore": "^1.0.10", |
There was a problem hiding this comment.
This is probably a dev dependency
There was a problem hiding this comment.
Removed on new PR
| import Box from "@cloudscape-design/components/box"; | ||
| import clsx from "clsx"; | ||
| import { CodeViewProps } from "./interfaces"; | ||
| import * as classes from "./styles.css"; |
There was a problem hiding this comment.
We usually call this styles in our main repository. Would be nice to be consistent.
There was a problem hiding this comment.
Updated on new PR
|
|
||
| return ( | ||
| <div | ||
| className={clsx( |
There was a problem hiding this comment.
I think you also need to call getBaseProps here to pull in the className and id attributes.
There was a problem hiding this comment.
Done on new PR
| // Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved. | ||
| // SPDX-License-Identifier: Apache-2.0 | ||
| import * as tokens from "@cloudscape-design/design-tokens"; | ||
| import { createGlobalTheme, createThemeContract, style } from "@vanilla-extract/css"; |
There was a problem hiding this comment.
Can't we just use sass? I would rather keep the frameworks consistent across our repositories.
There was a problem hiding this comment.
Updated on new PR to align with the other components
| import Popover from "@cloudscape-design/components/popover"; | ||
| import StatusIndicator from "@cloudscape-design/components/status-indicator"; | ||
| import { useState } from "react"; | ||
|
|
There was a problem hiding this comment.
We cannot un-publish public APIs! Let's talk about this.
| "build:environment": "node scripts/environment", | ||
| "build:copy": "cp README.md dist/", | ||
| "build:index": "cp src/index.* dist", | ||
| "build:highlight": "sass src/highlight/theme.scss dist/highlight/theme.css && tsc -p src/highlight/tsconfig.json", |
There was a problem hiding this comment.
I think we're using the ace theme now, right?
There was a problem hiding this comment.
Simplified this.
| "build:highlight": "sass src/highlight/theme.scss dist/highlight/theme.css && tsc -p src/highlight/tsconfig.json", | ||
| "build:test": "node scripts/test-utils", | ||
| "build:rollup": "rollup --config rollup.config.js --bundleConfigAsCjs", | ||
| "build:tsc": "tsc --project ./demo/tsconfig.json", |
There was a problem hiding this comment.
Let's maybe call this build:demo? Otherwise it sounds like it would build the main bundle ts
There was a problem hiding this comment.
Aligned it with board-components.
| "prebuild": "rm -rf dist && mkdir dist", | ||
| "prepublishOnly": "npm run build", | ||
| "lint": "eslint --ignore-path .gitignore --ext ts,tsx,js --fix .", | ||
| "build": "npm-run-all build:*", |
There was a problem hiding this comment.
Can all these build steps be executed in a random order?
| test("correctly renders component content", () => { | ||
| render(<CodeView content={"Hello World"}></CodeView>); | ||
| const wrapper = createWrapper().findCodeView()!; | ||
| expect(wrapper!.findContent().getElement().textContent).toBe("Hello World"); |
There was a problem hiding this comment.
minor: I prefer .toHaveTextContent over .textContent).toBe because it feels like the better abstraction.
There was a problem hiding this comment.
Updated in new PR
| expect(wrapper!.findLineNumbers()!.getElement().textContent).toBe("123"); | ||
| }); | ||
|
|
||
| test("correctly tokenizes content if highlight is set", () => { |
There was a problem hiding this comment.
Should this also test that it doesnt apply the default highlighting from ace?
There was a problem hiding this comment.
Will extend these tests on new PR
| colorBackgroundCode: null, | ||
| }); | ||
| createGlobalTheme(":root", vars, { | ||
| colorBackgroundCode: "#f4f4f4", |
There was a problem hiding this comment.
Can we use design tokens please?
There was a problem hiding this comment.
Will add this design token
|
|
||
| export const code = style({ | ||
| backgroundColor: vars.colorBackgroundCode, | ||
| padding: "8px", |
There was a problem hiding this comment.
Can we use design tokens please?
There was a problem hiding this comment.
Used on new PR
| html:not(.awsui-dark-mode) body:not(.awsui-dark-mode) { | ||
| @import "../../node_modules/ace-code/styles/theme/cloud_editor"; | ||
| .ace-cloud_editor { | ||
| background: none; |
There was a problem hiding this comment.
I wonder if there's any way to use the theme as-is, without this overwrite
There was a problem hiding this comment.
Our background color is very slightly different from the included one, so we need this.
|
Adressed most (if not all of the comments here). Opened a new PR: #5 |
Added the initial source the code-view.