-
Notifications
You must be signed in to change notification settings - Fork 4
feat: added prototype code to make ui grey and present a banner to user #39
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Large diffs are not rendered by default.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -7,14 +7,26 @@ | |
| "publishConfig": { | ||
| "access": "public" | ||
| }, | ||
| "main": "dist/cli.js", | ||
| "main": "dist/index.js", | ||
| "types": "dist/index.d.ts", | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Package exports declare .d.ts files but TypeScript config may not generate them. If tsconfig.json missing "declaration": true", build produces no type definitions → consumers get type errors. Recommendation:
Why matters: Consumers importing @patternfly/patternfly-cli in TypeScript projects will get "Cannot find module" errors without type definitions, even though runtime works. |
||
| "type": "module", | ||
| "bin": { | ||
| "patternfly-cli": "./dist/cli.js", | ||
| "pf": "./dist/cli.js" | ||
| }, | ||
| "exports": { | ||
| ".": { | ||
| "types": "./dist/index.d.ts", | ||
| "import": "./dist/index.js" | ||
| }, | ||
| "./components": { | ||
| "types": "./dist/index.d.ts", | ||
| "import": "./dist/index.js" | ||
| }, | ||
| "./prototype.css": "./dist/prototype.css" | ||
| }, | ||
| "scripts": { | ||
| "build": "tsc", | ||
| "build": "tsc && cp src/prototype.css dist/prototype.css", | ||
| "start": "node dist/cli.js", | ||
| "test": "jest", | ||
| "semantic-release": "semantic-release", | ||
|
|
@@ -55,10 +67,17 @@ | |
| "commander": "^12.1.0", | ||
| "execa": "^9.3.0", | ||
| "fs-extra": "^11.2.0", | ||
| "glob": "^11.0.0", | ||
| "inquirer": "^9.3.5" | ||
| }, | ||
| "peerDependencies": { | ||
| "react": "^18.0.0", | ||
| "react-dom": "^18.0.0", | ||
| "@patternfly/react-core": "^5.0.0 || ^6.0.0" | ||
| }, | ||
| "devDependencies": { | ||
| "@eslint/js": "^10.0.1", | ||
| "@patternfly/react-core": "^6.4.3", | ||
| "@semantic-release/changelog": "^6.0.3", | ||
| "@semantic-release/commit-analyzer": "^13.0.1", | ||
| "@semantic-release/git": "^10.0.1", | ||
|
|
@@ -71,8 +90,12 @@ | |
| "@types/inquirer": "^9.0.9", | ||
| "@types/jest": "^30.0.0", | ||
| "@types/node": "^24.10.1", | ||
| "@types/react": "^18.3.0", | ||
| "@types/react-dom": "^18.3.0", | ||
| "eslint": "^10.0.2", | ||
| "jest": "^29.7.0", | ||
| "react": "^18.3.0", | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If these are in peerDependencies, they shouldn't also be needed in devDependencies. Same for @patternfly/react-core |
||
| "react-dom": "^18.3.0", | ||
| "semantic-release": "^24.2.0", | ||
| "ts-jest": "^29.4.5", | ||
| "typescript": "^5.9.3", | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,145 @@ | ||
| jest.mock('fs-extra', () => { | ||
| const real = jest.requireActual<typeof import('fs-extra')>('fs-extra'); | ||
| return { | ||
| __esModule: true, | ||
| default: { | ||
| pathExists: jest.fn(), | ||
| readFile: jest.fn(), | ||
| writeFile: jest.fn(), | ||
| existsSync: real.existsSync, | ||
| readFileSync: real.readFileSync, | ||
| }, | ||
| }; | ||
| }); | ||
|
|
||
| jest.mock('glob', () => ({ | ||
| __esModule: true, | ||
| glob: jest.fn(), | ||
| })); | ||
|
|
||
| import path from 'path'; | ||
| import fs from 'fs-extra'; | ||
| import { glob } from 'glob'; | ||
| import { runPrototype } from '../prototype.js'; | ||
|
|
||
|
Comment on lines
+1
to
+24
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
# Verify whether inquirer is mocked in this test file
rg -n "jest\\.mock\\('inquirer'|inquirer\\.prompt|runPrototype\\(" src/__tests__/prototype.test.tsRepository: patternfly/patternfly-cli Length of output: 378 🏁 Script executed: # Find the prototype implementation file
fd -e ts -e js prototype | grep -v test | head -5Repository: patternfly/patternfly-cli Length of output: 86 🏁 Script executed: # Read the full test file to see the current setup
cat -n src/__tests__/prototype.test.ts | head -50Repository: patternfly/patternfly-cli Length of output: 2204 🏁 Script executed: # Check the runPrototype implementation
find . -name "prototype.ts" -o -name "prototype.js" | grep -v test | xargs cat 2>/dev/null | head -80Repository: patternfly/patternfly-cli Length of output: 2206 🏁 Script executed: # Get more of the prototype.ts file to see if runPrototype calls inquirer.prompt
cat -n src/prototype.ts | grep -A 5 -B 5 "inquirer\|async function runPrototype"Repository: patternfly/patternfly-cli Length of output: 1200 🏁 Script executed: # Check if tests actually run or fail without the inquirer mock
tail -100 src/__tests__/prototype.test.ts | head -50Repository: patternfly/patternfly-cli Length of output: 2532 Mock
Proposed fix+jest.mock('inquirer', () => ({
+ __esModule: true,
+ default: {
+ prompt: jest.fn(),
+ },
+}));
...
import path from 'path';
import fs from 'fs-extra';
import { glob } from 'glob';
+import inquirer from 'inquirer';
import { runPrototype } from '../prototype.js';
...
const mockGlob = glob as jest.MockedFunction<typeof glob> & jest.Mock;
+const mockPrompt = inquirer.prompt as jest.MockedFunction<typeof inquirer.prompt> & jest.Mock;
...
beforeEach(() => {
jest.clearAllMocks();
+ mockPrompt.mockResolvedValue({ bannerMessage: '' });
// Suppress console.log and console.error during tests
jest.spyOn(console, 'log').mockImplementation();
jest.spyOn(console, 'error').mockImplementation();
});🤖 Prompt for AI Agents
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I agree with this: Test coverage incomplete (src/tests/prototype.test.ts)
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
+jest.mock('inquirer', () => ({
+ __esModule: true,
+ default: {
+ prompt: jest.fn(),
+ },
+}));
+
jest.mock('fs-extra', () => {
...
import path from 'path';
import fs from 'fs-extra';
import { glob } from 'glob';
+import inquirer from 'inquirer';
import { runPrototype } from '../prototype.js';
...
const mockGlob = glob as jest.MockedFunction<typeof glob> & jest.Mock;
+const mockPrompt = inquirer.prompt as jest.MockedFunction<typeof inquirer.prompt> & jest.Mock;
...
beforeEach(() => {
jest.clearAllMocks();
+ mockPrompt.mockResolvedValue({ bannerMessage: '' });
// Suppress console.log and console.error during testsThe ᓚᘏᗢ ✨ |
||
| const mockPathExists = fs.pathExists as jest.MockedFunction<typeof fs.pathExists> & jest.Mock; | ||
| const mockReadFile = fs.readFile as jest.MockedFunction<typeof fs.readFile> & jest.Mock; | ||
| const mockWriteFile = fs.writeFile as jest.MockedFunction<typeof fs.writeFile> & jest.Mock; | ||
| const mockGlob = glob as jest.MockedFunction<typeof glob> & jest.Mock; | ||
|
|
||
| describe('runPrototype', () => { | ||
| const testCwd = '/test/project'; | ||
| const CSS_IMPORT = "import '@patternfly/patternfly-cli/prototype.css';"; | ||
|
|
||
| beforeEach(() => { | ||
| jest.clearAllMocks(); | ||
| // Suppress console.log and console.error during tests | ||
| jest.spyOn(console, 'log').mockImplementation(); | ||
| jest.spyOn(console, 'error').mockImplementation(); | ||
| }); | ||
|
|
||
| afterEach(() => { | ||
| jest.restoreAllMocks(); | ||
| }); | ||
|
|
||
| it('should find and modify src/index.tsx', async () => { | ||
| const indexPath = path.join(testCwd, 'src/index.tsx'); | ||
| const originalContent = `import React from 'react';\nimport ReactDOM from 'react-dom';\n\nReactDOM.render(<App />, document.getElementById('root'));`; | ||
| const expectedContent = `import React from 'react';\nimport ReactDOM from 'react-dom';\n${CSS_IMPORT}\n\nReactDOM.render(<App />, document.getElementById('root'));`; | ||
|
|
||
| mockPathExists.mockResolvedValue(true); | ||
| mockReadFile.mockResolvedValue(originalContent); | ||
| mockWriteFile.mockResolvedValue(undefined); | ||
|
|
||
| await runPrototype(testCwd); | ||
|
|
||
| expect(mockPathExists).toHaveBeenCalledWith(indexPath); | ||
| expect(mockReadFile).toHaveBeenCalledWith(indexPath, 'utf-8'); | ||
| expect(mockWriteFile).toHaveBeenCalledWith(indexPath, expectedContent, 'utf-8'); | ||
| }); | ||
|
|
||
| it('should find and modify src/index.jsx', async () => { | ||
| const tsxPath = path.join(testCwd, 'src/index.tsx'); | ||
| const jsxPath = path.join(testCwd, 'src/index.jsx'); | ||
| const originalContent = `import React from 'react';\n\nReactDOM.render(<App />, document.getElementById('root'));`; | ||
|
|
||
| mockPathExists | ||
| .mockResolvedValueOnce(false) // src/index.tsx doesn't exist | ||
| .mockResolvedValueOnce(true); // src/index.jsx exists | ||
| mockReadFile.mockResolvedValue(originalContent); | ||
| mockWriteFile.mockResolvedValue(undefined); | ||
|
|
||
| await runPrototype(testCwd); | ||
|
|
||
| expect(mockPathExists).toHaveBeenCalledWith(tsxPath); | ||
| expect(mockPathExists).toHaveBeenCalledWith(jsxPath); | ||
| expect(mockReadFile).toHaveBeenCalledWith(jsxPath, 'utf-8'); | ||
| }); | ||
|
|
||
| it('should not modify file if import already exists', async () => { | ||
| const indexPath = path.join(testCwd, 'src/index.tsx'); | ||
| const contentWithImport = `import React from 'react';\n${CSS_IMPORT}\nimport ReactDOM from 'react-dom';\n\nReactDOM.render(<App />, document.getElementById('root'));`; | ||
|
|
||
| mockPathExists.mockResolvedValue(true); | ||
| mockReadFile.mockResolvedValue(contentWithImport); | ||
|
|
||
| await runPrototype(testCwd); | ||
|
|
||
| expect(mockReadFile).toHaveBeenCalledWith(indexPath, 'utf-8'); | ||
| expect(mockWriteFile).not.toHaveBeenCalled(); | ||
| }); | ||
|
|
||
| it('should add import at the beginning if no imports exist', async () => { | ||
| const indexPath = path.join(testCwd, 'src/index.tsx'); | ||
| const originalContent = `const app = document.getElementById('root');\napp.innerHTML = 'Hello';`; | ||
| const expectedContent = `${CSS_IMPORT}\nconst app = document.getElementById('root');\napp.innerHTML = 'Hello';`; | ||
|
|
||
| mockPathExists.mockResolvedValue(true); | ||
| mockReadFile.mockResolvedValue(originalContent); | ||
| mockWriteFile.mockResolvedValue(undefined); | ||
|
|
||
| await runPrototype(testCwd); | ||
|
|
||
| expect(mockWriteFile).toHaveBeenCalledWith(indexPath, expectedContent, 'utf-8'); | ||
| }); | ||
|
|
||
| it('should throw error if no index file is found', async () => { | ||
| mockPathExists.mockResolvedValue(false); | ||
| mockGlob.mockResolvedValue([]); | ||
|
|
||
| await expect(runPrototype(testCwd)).rejects.toThrow('Main index file not found'); | ||
|
|
||
| expect(mockWriteFile).not.toHaveBeenCalled(); | ||
| }); | ||
|
|
||
| it('should use glob to find index file if common locations do not exist', async () => { | ||
| const foundIndexPath = path.join(testCwd, 'app/index.tsx'); | ||
| const originalContent = `import React from 'react';\n\nfunction App() { return <div>Hello</div>; }`; | ||
|
|
||
| mockPathExists.mockResolvedValue(false); | ||
| mockGlob.mockResolvedValue([foundIndexPath]); | ||
| mockReadFile.mockResolvedValue(originalContent); | ||
| mockWriteFile.mockResolvedValue(undefined); | ||
|
|
||
| await runPrototype(testCwd); | ||
|
|
||
| expect(mockGlob).toHaveBeenCalled(); | ||
| expect(mockReadFile).toHaveBeenCalledWith(foundIndexPath, 'utf-8'); | ||
| expect(mockWriteFile).toHaveBeenCalled(); | ||
| }); | ||
|
|
||
| it('should prefer src directory when multiple index files are found', async () => { | ||
| const srcIndexPath = path.join(testCwd, 'src/index.tsx'); | ||
| const otherIndexPath = path.join(testCwd, 'other/index.tsx'); | ||
| const originalContent = `import React from 'react';`; | ||
|
|
||
| mockPathExists.mockResolvedValue(false); | ||
| mockGlob.mockResolvedValue([otherIndexPath, srcIndexPath]); | ||
| mockReadFile.mockResolvedValue(originalContent); | ||
| mockWriteFile.mockResolvedValue(undefined); | ||
|
|
||
| await runPrototype(testCwd); | ||
|
|
||
| expect(mockReadFile).toHaveBeenCalledWith(srcIndexPath, 'utf-8'); | ||
| }); | ||
| }); | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,17 @@ | ||
| import * as React from "react"; | ||
| import { Banner, Bullseye } from "@patternfly/react-core"; | ||
|
|
||
| export interface ProtoProps { | ||
| message?: string; | ||
| } | ||
| const ProtoBanner: React.FC<ProtoProps> = ({ message = "This application is a design prototype"}) => { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. should this be exported? |
||
| return ( | ||
| <Banner isSticky> | ||
| <Bullseye> | ||
| <strong>{message}</strong> | ||
| </Bullseye> | ||
| </Banner> | ||
| ); | ||
| }; | ||
|
|
||
| export default ProtoBanner; | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,2 @@ | ||
| // Export React components | ||
| export { default as ProtoBanner } from './components/protoBanner.js'; |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,3 @@ | ||
| html { | ||
| filter: grayscale(100%); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You don't want to include this in the PR. I'd add it to gitignore