feat(docgen-sidebar): support new types for pdf template#4513
feat(docgen-sidebar): support new types for pdf template#4513myu-box wants to merge 3 commits intobox:masterfrom
Conversation
WalkthroughThe PR extends the DocGenSidebar component to support PDF form field tags (checkbox, radiobutton, dropdown) by refactoring the sidebar to dynamically organize tags into sections. The component now groups tags by category—text, image, and per-field type—instead of using hardcoded sections. Type definitions are extended with new tag types, a required field, and helper functions. Mock data and localization messages are added to support testing and rendering. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/elements/content-sidebar/DocGenSidebar/types.ts (1)
4-13: Model unknown API tag types instead of casting around the union.The sidebar and tests intentionally support unknown
tag_typevalues falling back to Text tags, butDocGenTag['tag_type']currently excludes them. That forcesas unknown as DocGenTagin tests and makes the API contract narrower than the runtime behavior.Suggested type adjustment
+export const KNOWN_DOC_GEN_TAG_TYPES = [ + 'text', + 'arithmetic', + 'conditional', + 'for-loop', + 'table-loop', + 'image', + 'checkbox', + 'radiobutton', + 'dropdown', +] as const; + +export type KnownDocGenTagType = (typeof KNOWN_DOC_GEN_TAG_TYPES)[number]; + // our apis are in snake case export type DocGenTag = { /* eslint-disable-next-line camelcase */ - tag_type: - | 'text' - | 'arithmetic' - | 'conditional' - | 'for-loop' - | 'table-loop' - | 'image' - | 'checkbox' - | 'radiobutton' - | 'dropdown'; + tag_type: KnownDocGenTagType | (string & {}); /* eslint-disable-next-line camelcase */ tag_content: string; /* eslint-disable-next-line camelcase */ json_paths: Array<string>; required: boolean; @@ /** PDF template control tags that render in their own sidebar section. */ export const PDF_FIELD_TAG_TYPES = ['checkbox', 'radiobutton', 'dropdown'] as const; +export type PdfFieldTagType = (typeof PDF_FIELD_TAG_TYPES)[number]; -export function isPdfFormFieldTagType(tagType: DocGenTag['tag_type']): boolean { - return (PDF_FIELD_TAG_TYPES as readonly DocGenTag['tag_type'][]).includes(tagType); +export function isPdfFormFieldTagType(tagType: DocGenTag['tag_type']): tagType is PdfFieldTagType { + return (PDF_FIELD_TAG_TYPES as readonly string[]).includes(tagType); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/elements/content-sidebar/DocGenSidebar/types.ts` around lines 4 - 13, DocGenTag['tag_type'] currently lists only known literals, which prevents representing unknown API tag types and forces casts in tests; update the union in types.ts (the tag_type definition) to allow arbitrary strings (e.g., append | string to the existing union) so unknown tag_type values are accepted and code/tests can rely on runtime fallback to text without using as unknown as DocGenTag casts.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/elements/content-sidebar/DocGenSidebar/types.ts`:
- Around line 4-13: DocGenTag['tag_type'] currently lists only known literals,
which prevents representing unknown API tag types and forces casts in tests;
update the union in types.ts (the tag_type definition) to allow arbitrary
strings (e.g., append | string to the existing union) so unknown tag_type values
are accepted and code/tests can rely on runtime fallback to text without using
as unknown as DocGenTag casts.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a4a727c1-b8ec-41f8-a27b-17a20fd824c4
📒 Files selected for processing (6)
src/elements/content-sidebar/DocGenSidebar/DocGenSidebar.tsxsrc/elements/content-sidebar/DocGenSidebar/messages.tsxsrc/elements/content-sidebar/DocGenSidebar/stories/DocGenSidebar.stories.tsxsrc/elements/content-sidebar/DocGenSidebar/types.tssrc/elements/content-sidebar/__mocks__/DocGenSidebar.mock.tssrc/elements/content-sidebar/__tests__/DocGenSidebar.test.tsx
| tag_content: string; | ||
| /* eslint-disable-next-line camelcase */ | ||
| json_paths: Array<string>; | ||
| required: boolean; |
There was a problem hiding this comment.
thanks for adding :) but since required is a required type property now, does that cause any sort of breaking changes? just want to check and raise - trust you and polina on the logic of all things docgen.
There was a problem hiding this comment.
The required property follows the DocGen tag type format to keep consistency with the type definition. At the moment, we are not using this required field in the DocGen Sidebar for any UI behavior changes.
| return ( | ||
| <SidebarContent sidebarView={SIDEBAR_VIEW_DOCGEN} title={formatMessage(messages.docGenTags)}> | ||
| <div className={classNames('bcs-DocGenSidebar', { center: isEmpty || hasError || isLoading })}> | ||
| {hasError && <Error onClick={() => loadTags(DEFAULT_RETRIES)} />} |
There was a problem hiding this comment.
Unrelated to your PR, but reading the additions at
https://github.com/box/box-ui-elements/pull/4513/changes#diff-89a3e1cb69853b3b87f5751630234d05ffc288ba9c9e3cb35f918ced1f435db3R115-R117
made me think of this.
It looks like it is theoretically possible to be in both an error state and a loading state and have both show up? it may not be possible (i dont know for sure!) and don't address in this PR, but... if your project leads you back to this file in the future might be something to look at.
There was a problem hiding this comment.
Good catch. I’ll watch for it in preview. If it shows up in practice We need to put up a follow-up PR. Thanks for calling it out
jpan-box
left a comment
jpan-box
left a comment
There was a problem hiding this comment.
LGTM from structural perspective (no significant changes to buie framework or cross element interactions) and general BUIE/box coding practices.
rely on you and teammate +1 for specific code logic! thank you so much for your work and revisions!
2 participants
This PR adds support for new PDF form field types:
checkbox,radiobutton, anddropdown.Previously, all PDF form fields were grouped under the
texttags section. This update introduces new sections for these new field types.Changes
checkbox,radiobuttonanddropdownfield typeSummary by CodeRabbit
Release Notes
New Features
Tests