-
Notifications
You must be signed in to change notification settings - Fork 23
feat(RichTextEditor): allow schema controls without toolbar buttons #7021
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 4 commits
1cf37de
a1d89e0
94024dd
8ebf6eb
27831f1
8949205
7ea1ddc
5e79797
0ada889
05ca1ee
77e4b97
b53461f
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,7 @@ | ||
| --- | ||
| '@kaizen/components': minor | ||
| --- | ||
|
|
||
| feat(RichTextEditor): allow schema controls without toolbar buttons via `showInToolbar` | ||
|
|
||
| Also even out the field border when the toolbar wrapper is empty. | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -53,6 +53,10 @@ | |
|
|
||
| .toolbarWrapper { | ||
| border-bottom: inherit; | ||
|
|
||
| &:empty { | ||
|
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. Convention: no new additions to AGENTS.md §3 and REVIEW.md both state: "Existing SCSS ( The fix itself is correct, but per project convention it should live in a .toolbarWrapper:empty {
border-bottom: none;
}and compose/merge the class names in the component. Alternatively, if the team considers this too heavyweight for a 3-line patch fix, an explicit exception is worth noting in the PR discussion.
Contributor
Author
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. Fixed by 7ea1ddc Removed the :empty addition from the legacy SCSS and apply toolbarWrapper only when controls are toolbar-visible. Comment and fix generated by Claude Code |
||
| border-bottom: none; | ||
| } | ||
|
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. Convention violation: AGENTS.md and REVIEW.md prohibit adding new rules to existing The
Ref: REVIEW.md — "No new
Contributor
Author
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. Fixed by 7ea1ddc Same fix as the earlier SCSS convention thread — no new SCSS was kept. Comment and fix generated by Claude Code |
||
| } | ||
|
|
||
| /* stylelint-disable no-descending-specificity */ | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -204,3 +204,19 @@ describe('RTE receives list controls', () => { | |
| }) | ||
| }) | ||
| }) | ||
|
|
||
| describe('showInToolbar', () => { | ||
| it('omits toolbar buttons for controls with showInToolbar false', () => { | ||
| render( | ||
| <TestRTE | ||
| controls={[ | ||
| { name: 'bold', group: 'inline', showInToolbar: false }, | ||
| { name: 'italic', group: 'inline' }, | ||
| ]} | ||
| />, | ||
| ) | ||
|
|
||
| expect(screen.queryByRole('button', { name: 'Bold' })).not.toBeInTheDocument() | ||
| expect(screen.getByRole('button', { name: 'Italic' })).toBeInTheDocument() | ||
| }) | ||
| }) | ||
|
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. Testing posture: This test follows the "does the component render X when prop Y is set" pattern, which AGENTS.md §8 and REVIEW.md direct to stickersheet or Consider moving this assertion to a Ref: REVIEW.md — "Reject 'does the component render X when prop Y is set' as a unit test — that is a stickersheet or spec story job."
Contributor
Author
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. Fixed by 0ada889 Comment and fix generated by Claude Code |
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -162,7 +162,9 @@ export const RichTextEditor = ({ | |
| styles.editor, | ||
| styles[`rows${rows}`], | ||
| classNameOverride, | ||
| controls != null && controls.length > 0 && styles.hasToolbar, | ||
| controls != null && | ||
| controls.some((control) => control.showInToolbar !== false) && | ||
|
Contributor
Author
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. nit: Make this a variable with a clear name
Contributor
Author
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. Fixed by 27831f1 Comment and fix generated by Claude Code |
||
| styles.hasToolbar, | ||
| )} | ||
| {...restProps} | ||
| /> | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -97,6 +97,38 @@ export const ControlsWithoutBold: Story = { | |
| }, | ||
| } | ||
|
|
||
| export const SchemaOnlyBold: Story = { | ||
| args: { | ||
| controls: [ | ||
| { name: 'bold', group: 'inline', showInToolbar: false }, | ||
| { name: 'italic', group: 'inline' }, | ||
| { name: 'underline', group: 'inline' }, | ||
| ], | ||
| defaultValue: [ | ||
| { | ||
| type: 'paragraph', | ||
| content: [ | ||
| { | ||
| type: 'text', | ||
| marks: [{ type: 'strong' }], | ||
| text: 'Bold text can be pasted or loaded', | ||
| }, | ||
| { | ||
| type: 'text', | ||
| text: ' without a Bold toolbar button', | ||
| }, | ||
| ], | ||
| }, | ||
| ], | ||
| }, | ||
| } | ||
|
|
||
| export const NoControls: Story = { | ||
|
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. Convention: visual variants belong in a stickersheet REVIEW.md states: "New or changed visual components must have or extend a The doc story here is fine to keep for documentation, but adding a stickersheet would align with the project's Priority 1 testing convention and provide a dedicated visual-regression anchor.
Contributor
Author
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. Fixed by 5e79797 Comment and fix generated by Claude Code |
||
| args: { | ||
| controls: undefined, | ||
| }, | ||
| } | ||
|
|
||
| export const DefaultValue: Story = { | ||
| args: { | ||
| defaultValue: [ | ||
|
|
||
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.
nit: Maybe elaborate here a little bit.
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.
Fixed by 8949205
Comment and fix generated by Claude Code