feat(docgen-sidebar): support new types for pdf template#4508
feat(docgen-sidebar): support new types for pdf template#4508myu-box wants to merge 1 commit intobox:masterfrom
Conversation
|
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (6)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (3)
WalkthroughReplaces DocGenSidebar's fixed text/image state with a unified Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/elements/content-sidebar/__tests__/DocGenSidebar.test.tsx (1)
63-80: Strengthen the test to assert per-section tag placement.The new test confirms the four headers render, but not that
NameField,SubscribeCheckbox,Gender, andCountryDropdownare grouped under their matching sections. Adding scoped tag assertions would better protect the core behavior of this PR.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/elements/content-sidebar/__tests__/DocGenSidebar.test.tsx` around lines 63 - 80, Update the test 'should render PDF template tags in separate sections' to assert that each tag appears inside its correct section: after rendering with mocked getDocGenTags and awaiting screen.findAllByTestId('bcs-TagsSection'), use the testing-library within(...) on each returned section element to assert that 'NameField' exists in the Text tags section, 'SubscribeCheckbox' exists in the Checkbox tags section, 'Gender' exists in the Radiobutton tags section, and 'CountryDropdown' exists in the Dropdown tags section (use renderComponent, mockPdfTemplateData and within from `@testing-library/react` to scope the queries).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/elements/content-sidebar/DocGenSidebar/DocGenSidebar.tsx`:
- Around line 70-103: buildDocGenSections currently creates sections when tags
exist but doesn't ensure those tags have non-empty json_paths, causing empty
sections to render; modify the logic in buildDocGenSections to only push a
DocGenSection when tagsToJsonPaths(...) returns a non-empty tree (or when at
least one tag has non-empty json_paths) for textTags, imageTags and each
fieldTags inside PDF_FIELD_TAG_TYPES, e.g., compute const tree =
tagsToJsonPaths(tags) and push the section only if tree.length > 0, using the
existing id/message/ tree shape for the section so isEmpty (which checks
sections.length) will behave correctly.
---
Nitpick comments:
In `@src/elements/content-sidebar/__tests__/DocGenSidebar.test.tsx`:
- Around line 63-80: Update the test 'should render PDF template tags in
separate sections' to assert that each tag appears inside its correct section:
after rendering with mocked getDocGenTags and awaiting
screen.findAllByTestId('bcs-TagsSection'), use the testing-library within(...)
on each returned section element to assert that 'NameField' exists in the Text
tags section, 'SubscribeCheckbox' exists in the Checkbox tags section, 'Gender'
exists in the Radiobutton tags section, and 'CountryDropdown' exists in the
Dropdown tags section (use renderComponent, mockPdfTemplateData and within from
`@testing-library/react` to scope the queries).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 04fd271f-3544-4d82-a12c-b10ccca526d5
📒 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
pyaromchyk-stack
left a comment
pyaromchyk-stack
left a comment
There was a problem hiding this comment.
LGTM from docgen side
| ), | ||
| }); | ||
|
|
||
| const tagList = await screen.findAllByTestId('bcs-TagsSection'); |
There was a problem hiding this comment.
is it completely necessary to use testIds? our general practice is to use semantic queries when possible like getByRole. just want to double check here - trusting your judgement.
There was a problem hiding this comment.
Correct. I followed the existing test style at first, but the behavior we care about is the rendered labels, so findByText (and getByRole when it applies) fit better than testid.
I removed the toHaveLength(4) . Besides, also added a new test that an unknown tag_type appears under text tag section.
| tag_type: 'text', | ||
| tag_content: '{{NameField::optional}}', | ||
| json_paths: ['NameField'], | ||
| required: false, |
There was a problem hiding this comment.
I notice all the other objects in this page have tag_type, tag_content, and json_paths. where does required come from?
I am guessing the type for this object is defined here: https://github.com/box/box-ui-elements/blob/master/src/elements/content-sidebar/DocGenSidebar/types.ts#L2
which also does not have required. just want to make sure the types match up correctly here
There was a problem hiding this comment.
Good catch!! The property already exists but was missing in type.ts. Updated.
| dropdown: messages.dropdownTags, | ||
| }; | ||
|
|
||
| const createNestedObject = (base: JsonPathsMap, paths: string[]) => { |
There was a problem hiding this comment.
naming nitpick: this function is called createNestedObject, but as I understand it .reduce doesn't so much create a new object as much as it modifies an existing one.
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Array/reduce
I feel like something like populateNestedPaths or addNestedPaths would better communicate the mutation to future readers. What do you think?
There was a problem hiding this comment.
combined these logic into one buildJsonPathTree function
src/elements/content-sidebar/DocGenSidebar/DocGenSidebar.tsx
| return obj[path]; | ||
| }, base); | ||
| }; | ||
| const tagsToJsonPaths = (docGenTags: DocGenTag[]): JsonPathsMap => { |
There was a problem hiding this comment.
wondering if this function will need to be used as a util in a future pr? ie should this be factored out into a utils file?
There was a problem hiding this comment.
combined these logic into one buildJsonPathTree function
src/elements/content-sidebar/DocGenSidebar/DocGenSidebar.tsx
| }); | ||
|
|
||
| const tagList = await screen.findAllByTestId('bcs-TagsSection'); | ||
| expect(tagList).toHaveLength(4); |
There was a problem hiding this comment.
I don't know how important this is, but since you have checked to see if there is a length of 4 with specific mock data, are there cases where it should return 3,2,1, or even 0 or invalid tag types? I have little to no context on what the api shape is like, i'm only thinking from a testing depth perspective. maybe some sort of parameterized test to cover all the cases.
There was a problem hiding this comment.
Correct. I followed the existing test style at first, but the behavior we care about is the rendered labels, so findByText (and getByRole when it applies) fit better than testid.
I removed the toHaveLength(4) . Besides, also added a new test that an unknown tag_type appears under text tag section.
| loadTags.call(this, attempts - 1); | ||
| } else if (response?.data) { | ||
| const { data } = response; | ||
| // anything that is not an image tag for this view is treated as a text tag |
There was a problem hiding this comment.
is this comment still helpful to keep?
There was a problem hiding this comment.
thanks for reminder! I moved to this line
src/elements/content-sidebar/DocGenSidebar/DocGenSidebar.tsx
|
Closing this PR in favor of a new one with the correct branch name. Please continue review here: #4513 |
4 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
New Features
Localization
Tests
Chores