[WEB-2283] fix: create issue modal parent select ui #5980
Conversation
anmolsinghbhatia
requested review from
SatishGandham,
rahulramesha and
sriramveeraghanta
WalkthroughThe changes in this pull request involve modifications to the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![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
🧹 Outside diff range and nitpick comments (2)
web/core/components/issues/issue-modal/components/default-properties.tsx (2)
265-320: LGTM! The parent issue selection UI improvements enhance consistency and usability.The changes effectively address the UI issues by:
- Maintaining consistent height (h-7) across all form elements
- Providing clear visual feedback with hover states
- Using semantic button elements with proper type attributes
- Preserving functionality while improving the visual structure
Consider adding
aria-labelto improve accessibility:<button type="button" + aria-label="Add parent issue" className="flex cursor-pointer items-center justify-between gap-1 h-full rounded border-[0.5px] border-custom-border-300 px-2 py-0.5 text-xs hover:bg-custom-background-80" onClick={() => setParentIssueListModalOpen(true)} >
267-309: Enhance CustomMenu accessibility for keyboard users.While the CustomMenu implementation is solid, consider these accessibility improvements:
<CustomMenu customButton={ <button type="button" + aria-label={`Current parent issue: ${selectedParentIssue?.project__identifier}-${selectedParentIssue?.sequence_id}`} + aria-haspopup="menu" className="flex cursor-pointer items-center justify-between gap-1 h-full rounded border-[0.5px] border-custom-border-300 px-2 py-0.5 text-xs hover:bg-custom-background-80" >
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
web/core/components/inbox/modals/create-modal/issue-properties.tsx(5 hunks)web/core/components/issues/issue-modal/components/default-properties.tsx(1 hunks)
🔇 Additional comments (2)
web/core/components/inbox/modals/create-modal/issue-properties.tsx (2)
Line range hint 174-231: LGTM! UI improvements enhance visual consistency.
The changes effectively improve the parent issue selection UI by:
- Maintaining consistent height with other form elements
- Ensuring proper button padding and alignment
- Utilizing full height in the CustomMenu component
94-94: Verify the impact of ignoring setIsOpen state management.
The empty arrow function () => {} for setIsOpen suggests that we're ignoring the modal's open state management. While this might be intentional if the state is managed internally, please verify that this doesn't break any functionality or accessibility features.
✅ Verification successful
Empty setIsOpen function is acceptable in this context
After analyzing the codebase, the empty arrow function () => {} for setIsOpen in IssueLabelSelect is acceptable because:
-
The
IssueLabelSelectcomponent is used in two different contexts:- In issue-modal where it requires active state management (
setIsOpen={setLabelModal}) - In create-modal where state management is not needed (our case with
setIsOpen={() => {}})
- In issue-modal where it requires active state management (
-
Looking at the component definition in
web/core/components/issues/select/label.tsx,setIsOpenis only used for handling the "Add new label" action, which is an optional feature not required in the create-modal context.
The implementation is consistent with React patterns where optional state management can be handled with no-op functions when the feature is not needed in a particular context.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other usages of IssueLabelSelect to verify if setIsOpen is used meaningfully elsewhere
ast-grep --pattern 'IssueLabelSelect $$$setIsOpen={$_}$$$'
Length of output: 60
Script:
#!/bin/bash
# Let's first find the IssueLabelSelect component definition
ast-grep --pattern 'export $_ function IssueLabelSelect' -l
# Also search for any file containing IssueLabelSelect
rg "IssueLabelSelect" --type typescript --type tsx -C 2
Length of output: 258
Script:
#!/bin/bash
# Search for IssueLabelSelect component definition and usage
rg "IssueLabelSelect" -t ts -t tsx -C 3
# Find the file that might contain the component
fd "IssueLabelSelect" --extension tsx --extension ts
Length of output: 120
Script:
#!/bin/bash
# Search for IssueLabelSelect using basic grep with context
grep -r "IssueLabelSelect" . --include="*.tsx" --include="*.ts" -C 3
# Also look for any modal-related components that might use setIsOpen
grep -r "setIsOpen" . --include="*.tsx" --include="*.ts" -C 2
Length of output: 56597
| <Controller | ||
| control={control} | ||
| name="parent_id" | ||
| render={({ field: { onChange } }) => ( | ||
| <CustomMenu.MenuItem | ||
| className="!p-1" | ||
| onClick={() => { | ||
| onChange(null); | ||
| handleFormChange(); | ||
| }} | ||
| > | ||
| Remove parent issue | ||
| </CustomMenu.MenuItem> | ||
| )} | ||
| /> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider consolidating parent_id Controllers.
There are two separate Controllers for the parent_id field. Consider refactoring to use a single Controller to improve maintainability:
- <Controller
- control={control}
- name="parent_id"
- render={({ field: { onChange } }) => (
- <CustomMenu.MenuItem
- className="!p-1"
- onClick={() => {
- onChange(null);
- handleFormChange();
- }}
- >
- Remove parent issue
- </CustomMenu.MenuItem>
- )}
- />
<Controller
control={control}
name="parent_id"
render={({ field: { onChange } }) => (
+ <>
+ {parentId && (
+ <CustomMenu.MenuItem
+ className="!p-1"
+ onClick={() => {
+ onChange(null);
+ handleFormChange();
+ }}
+ >
+ Remove parent issue
+ </CustomMenu.MenuItem>
+ )}
<ParentIssuesListModal
isOpen={parentIssueListModalOpen}
handleClose={() => setParentIssueListModalOpen(false)}
onChange={(issue) => {
onChange(issue.id);
handleFormChange();
setSelectedParentIssue(issue);
}}
projectId={projectId ?? undefined}
issueId={isDraft ? undefined : id}
/>
+ </>
)}
/>Also applies to: 321-341
Changes:
This PR includes fix for create issue modal parent select ui.
Reference:
[WEB-2283]
Media:
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Refactor