[wip] Add prompt library docs and update CLAUDE.md #3235
Conversation
…-progressive-disclosure
🗄️ Schema Change: No Changes ✅ |
|
Size Change: 0 B Total Size: 484 kB ℹ️ View Unchanged
|
🛠️ Item Splitting: No Changes ✅ |
npm Snapshot: PublishedGood news!! We've packaged up the latest commit from this PR (ab8a533) and published it to npm. You Example: pnpm add @khanacademy/perseus@PR3235If you are working in Khan Academy's frontend, you can run the below command. ./dev/tools/bump_perseus_version.ts -t PR3235If you are working in Khan Academy's webapp, you can run the below command. ./dev/tools/bump_perseus_version.js -t PR3235 |
There was a problem hiding this comment.
This document is what was used to find updated information for all these docs. It would not be part of the final merge. Will delete this after discussions. Kept for reference.
There was a problem hiding this comment.
This document is what was used to find updated information for all these docs. It would not be part of the final merge. Will delete this after discussions. Kept for reference.
There was a problem hiding this comment.
It may be possible to trim this file even more, but we do want some amount of info here.
| return <div>{props.content}</div>; | ||
| }; | ||
| ComponentName.displayName = "ComponentName"; // Extra step | ||
| ``` |
There was a problem hiding this comment.
I'm having trouble understanding the difference between these two examples. I understand that one is a named function, and the other is an arrow function, but I don't see the "Extra step" that is called out. I also don't know why the named function is the preferred form.
| optional?: boolean; | ||
| }; | ||
|
|
||
| // For complex props, use intersection types |
There was a problem hiding this comment.
Did we decide that we prefer interfaces when we export types? I don't recall if that impacts how we define complex props.
|
|
||
| function MyComponent() { | ||
| return ( | ||
| <View> |
There was a problem hiding this comment.
Perhaps this is more appropriate for a separate discussion: I would like to get away from using the <View> component as a standard "element". That component has some core styling associated with it (namely flexbox) that isn't always needed. Generally speaking, if we want a Perseus component to output elements that "need" a container with specific styling, then a <div> element (or other more semantically relevant element) with styling attached to it is my preference (simpler styling rather than trying to override <View>). However, if the component is contained within some other component that manages its allotted area, then my preference is to return the component content surrounded by React fragments (<> ... </>). Otherwise, if we promote using <View>, we end up with div-soup, which is cumbersome and inefficient. The <View> component has its place, but I think we should be judicious with it.
|
|
||
| // User answer state goes through handleUserInput, not local state! | ||
| } | ||
| ``` |
There was a problem hiding this comment.
Could you expand on why this is helpful for the AI? Doesn't it already know about state management?
| const filteredItems = React.useMemo( | ||
| () => items.filter(item => item.matches(filter)), | ||
| [items, filter] | ||
| ); |
There was a problem hiding this comment.
We may want to check in with Jeff Yates on the matter of useMemo. I believe he has some well-founded opinions on the matter of when (if ever) to use this feature.
|
|
||
| // ✅ Good - Stable reference | ||
| const style = {margin: 10}; | ||
| <Component style={style} /> |
There was a problem hiding this comment.
This instruction is combining two concepts: inline object definition and how to style. I would like to keep them separate. Is there a different inline object definition that could be used as an example? I don't want to give the AI any idea that it can provide styling inline.
| aria-describedby="hint-text" | ||
| aria-invalid={hasError} | ||
| /> | ||
| ``` |
There was a problem hiding this comment.
What is this telling the AI to do? If it is trying to enforce the use of labels/names on element, then I think this needs to be expanded significantly. My experience has been that the AI doesn't know when it is appropriate to add aria and when it isn't, nor how to properly generate a label.
|
|
||
| return <input ref={inputRef} />; | ||
| } | ||
| ``` |
There was a problem hiding this comment.
Is focus management something that we do in Perseus?
| break; | ||
| } | ||
| } | ||
| ``` |
There was a problem hiding this comment.
Does having this in the document encourage the AI to implement keyboard handling over native functionality?
| </View> | ||
| ); | ||
| } | ||
| ``` |
There was a problem hiding this comment.
I like this concept. However, I would prefer that it is handled via CSS, since that would work better in a wider variety of situations (i.e. zoomed viewport and increased font sizes).
| const {container} = render(<ComplexComponent {...props} />); | ||
| expect(container).toMatchSnapshot(); | ||
| }); | ||
| ``` |
There was a problem hiding this comment.
It would be cool if we could add guidance on when and how to generate visual regression tests, as well.
| if (error) return <ErrorMessage />; | ||
| return <Content />; | ||
| })()} | ||
| ``` |
There was a problem hiding this comment.
I would like to expand on this concept. I believe that we should be transitioning our hidden components instead of conditionally rendering them. In order to transition them (fade-in/fade-out, expand/collapse), we need the component rendered at all times, and CSS would determine how to show/hide them. This is also helpful for elements that control the visibility of other components (like an accordion). They need an aria-controls attribute that references an ID of an element that exists on the page. If that element is conditionally rendered, then it's a validation error that fails WCAG.
| ├── types.ts # Widget-specific types (if complex) | ||
| ├── utils.ts # Widget-specific utilities (if needed) | ||
| └── __docs__/ | ||
| ├── widget-name.stories.tsx # Storybook stories |
There was a problem hiding this comment.
Can we add in Visual Regression stories here, as well? (see Radio widget for examples)
|
|
||
| ### TypeScript/JavaScript Files | ||
| ```typescript | ||
| // Components - PascalCase file, named export |
There was a problem hiding this comment.
Shouldn't this be "kebab-case file"?
| ``` | ||
| widgets/widget-name/ | ||
| ├── index.ts # Public exports only | ||
| ├── widget-name.tsx # Main component |
There was a problem hiding this comment.
Should we also indicate CSS files in this structure?
| export const WithHint = {}; | ||
| export const Mobile = {}; | ||
| export const ErrorState = {}; | ||
| ``` |
There was a problem hiding this comment.
Is this another place where we want to call out visual regression stories?
| packages/perseus/src/ | ||
| ├── styles/ | ||
| │ ├── global.less # Global styles | ||
| │ └── variables.less # Shared variables |
There was a problem hiding this comment.
We don't use LESS anymore.
| │ └── variables.less # Shared variables | ||
| ├── widgets/ | ||
| │ └── widget-name/ | ||
| │ └── widget-name.less # Widget-specific styles |
There was a problem hiding this comment.
Widgets should be using CSS Modules: widget-name.module.css
| return ( | ||
| <input | ||
| value={userInput.value} | ||
| onChange={(e) => handleUserInput({value: e.target.value})} |
There was a problem hiding this comment.
Should this reflect the useCallback optimization that was presented in the best-practices file?
| ); | ||
|
|
||
| // After: O(n) | ||
| const hasDuplicate = items.length !== new Set(items).size; |
There was a problem hiding this comment.
This is a good example!
| 2. **Verify accessibility** - Use the a11y addon to catch issues | ||
| 3. **Check mobile layouts** - Use device frame addon | ||
| 4. **Debug interactions** - Use actions addon to log callbacks | ||
| 5. **Visual debugging** - Compare component states side-by-side |
There was a problem hiding this comment.
How does comparing "side-by-side" work here?
|
|
||
| # 6. Test in Storybook | ||
| pnpm storybook | ||
| ``` |
There was a problem hiding this comment.
Wasn't this called out in an earlier document?
| 1. Monitor for any reverted changes | ||
| 2. Check for related bug reports | ||
| 3. Watch performance metrics | ||
| 4. Respond to code review feedback |
There was a problem hiding this comment.
Are we expecting the AI to be doing these?
|
|
||
| ``` | ||
| packages/ | ||
| ├── perseus/ (v74.0.2) # Core rendering engine |
There was a problem hiding this comment.
suggestion add a section on our versioning instead of adding it here, since our version changes almost all the time
| │ │ ├── components/ # Reusable UI components | ||
| │ │ ├── renderers/ # Content renderers | ||
| │ │ ├── util/ # Utility functions | ||
| │ │ └── __docs__/ # Storybook documentation |
There was a problem hiding this comment.
currently widget documentation is under the specific widget, not sure how we want to represent that here.
| │ │ ├── renderers/ # Content renderers | ||
| │ │ ├── util/ # Utility functions | ||
| │ │ └── __docs__/ # Storybook documentation | ||
| │ └── __testdata__/ # Test fixtures |
There was a problem hiding this comment.
we also have the __tests__ folder that contains the test
|
|
||
| ## Migration Notes | ||
|
|
||
| - Radio widget is transitioning (TODO(LEMS-2994)) |
There was a problem hiding this comment.
This information will be outdated soon
|
|
||
| ## Three-Layer Type Architecture | ||
|
|
||
| Perseus widgets use a three-layer type system: |
There was a problem hiding this comment.
Do we need to reference this in here?
https://github.com/Khan/perseus/blob/main/__docs__/introduction.mdx
There was a problem hiding this comment.
I think some of my comments were not posted :(
Just a thought, if we can reference existing documentation links here for context or sample it will be better. It will be less maintenance and having information outdated.
| import type {PerseusWidgetNameUserInput} from "@khanacademy/perseus-core"; | ||
|
|
||
| // Props are just the widget options spread directly, plus system props | ||
| type Props = { |
There was a problem hiding this comment.
Based on previous discussion we would use an interface instead of the Props type and we would give it proper name for readability instead of just Props.
see sample here: https://github.com/Khan/perseus/blob/main/packages/perseus/src/widgets/radio/multiple-choice-component.tsx#L20
|
|
||
| // Export with WidgetExports interface | ||
| export default { | ||
| name: "widget-name" as const, |
There was a problem hiding this comment.
not sure if this would matter but widget names are in kebab case and the widget itself will be in upper camel case
| ### Type Definitions | ||
| ```typescript | ||
| // Place types at the top of the file | ||
| type Props = { |
There was a problem hiding this comment.
Base on discussion we would prefer interface than types see slack discussion here
|
|
||
| ## Wonder Blocks Integration | ||
|
|
||
| Perseus uses Khan Academy's Wonder Blocks design system: |
There was a problem hiding this comment.
What do you think of adding a context where wonder blocks is found https://khan.github.io/wonder-blocks by explicitly saying this here it will only search what available components based on what are installed in our packages, but there might be more to it available in the documentation.
| ### Deprecated Patterns | ||
| - `onChange` prop - Use `handleUserInput` instead (LEMS-3245, LEMS-3542) | ||
| - Local widget state for answers - Use UserInputManager | ||
| - Class components for new widgets - Use functional components |
There was a problem hiding this comment.
These here is a bit confusing to me. We can probably simplify these statements here, like:
More descriptive: "New widgets must use functional components, not classes"
Action-oriented: "Use functional components for new widgets"
|
|
||
| ### MathJax Configuration | ||
|
|
||
| Perseus uses MathJax for rendering. Key points: |
There was a problem hiding this comment.
Should this be something more like "Perseus uses MathJax for rendering math"? The implied context might be missed and I wouldn't want to see MathJax used in ways that it shouldn't.
| {"content": "The answer is $4$."} | ||
| ] | ||
| } | ||
| ``` |
There was a problem hiding this comment.
Should this be labelled as an example and not a template? Or, should there be a template here, instead?
| ```typescript | ||
| // Ensure widgets have proper labels | ||
| <TextField | ||
| aria-label="Enter the x-coordinate" |
There was a problem hiding this comment.
The preference would be to have a visible label for the text field that is associated with the input so that no aria-label is needed. I think this example should be removed in favor of a more expansive accessibility-focused document.
| - Consider pagination for content with 50+ expressions | ||
|
|
||
| ### Content Loading | ||
| - Large exercises should lazy-load hints |
There was a problem hiding this comment.
What constitutes as "large"?
| - [ ] Images load and align correctly | ||
| - [ ] Content is responsive on mobile | ||
| - [ ] Screen reader announces math properly | ||
| - [ ] Print view works correctly |
There was a problem hiding this comment.
What is this referencing? Are we supposed to consider printing when we style widgets?
| expect(result).toBe(expected); | ||
| }); | ||
|
|
||
| it("handles user interaction", async () => { |
There was a problem hiding this comment.
Maybe a different test title? I would expect that a "user interaction" would have an "Act" section.
| 2. **Accessibility** - Keyboard navigation, screen readers | ||
| 3. **Edge cases** - Empty states, invalid input, boundaries | ||
| 4. **Scoring logic** - Correct/incorrect answers | ||
| 5. **Mobile behavior** - Touch interactions |
There was a problem hiding this comment.
Maybe this is a side discussion, but I am confused as to what "Touch interactions" mean. I would expect that the standard onClick event would handle touches. What are we testing in a mobile environment?
| // In test files | ||
| expect(result).toHaveBeenAnsweredCorrectly(); | ||
| expect(result).toHaveInvalidInput("message"); | ||
| ``` |
There was a problem hiding this comment.
Perhaps it's because I'm not familiar with the custom matchers, but is it possible to have the result both "answered correctly" and "have invalid input"?
| render(<Widget />); | ||
| expect(screen.getByLabelText("Answer")).toBeInTheDocument(); | ||
| }); | ||
| ``` |
There was a problem hiding this comment.
I recommend removing this example in favor of a more detailed accessibility-focused document.
| render(<Widget />); | ||
|
|
||
| await user.pointer({keys: "[TouchA>]", target: element}); | ||
| // Assert touch-specific behavior |
There was a problem hiding this comment.
Is this a model that we are following - touch-specific behavior? I thought we were wanting to get away from mobile-specific anything.
| └── __docs__/ | ||
| ├── [widget-name].stories.tsx # Storybook stories | ||
| └── a11y.mdx # Accessibility docs | ||
| ``` |
There was a problem hiding this comment.
Do we need this here, again? It seems like it would be a pain to have to update this information in multiple locations whenever we make a change to directory structure.
| handleUserInput: (input: PerseusWidgetNameUserInput) => void; | ||
| trackInteraction: () => void; | ||
| onFocus: () => void; | ||
| onBlur: () => void; |
There was a problem hiding this comment.
Do we want to encourage outside containers telling the widget how to handle these kinds of events?
| focus: () => inputRef.current?.focus(), | ||
| blur: () => inputRef.current?.blur(), | ||
| })); | ||
| ``` |
There was a problem hiding this comment.
Same concern here about focus management.
There was a problem hiding this comment.
I'm just watching the session from last week, I wonder if we want to do this structure of claude.md per package, so it will only load the needed context when needed and will not overload claude context on the outset.
3 participants
Summary:
Shorten our CLAUDE.md by referencing sub-documents on specific topics. The goal is to use progressive disclosure to keep the main file short so context stays as relevant as possible. I will be testing these out on my next couple tickets to see if it works well, but would love feedback!