Conversation
|
Denis Marusevich seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account. You have signed the CLA already but the status is still pending? Let us recheck it. |
WalkthroughAdds an optional Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Important Action Needed: IP Allowlist UpdateIf your organization protects your Git platform with IP whitelisting, please add the new CodeRabbit IP address to your allowlist:
Reviews will stop working after February 8, 2026 if the new IP is not added to your allowlist. 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 |
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/features/unified-share-modal/EmailForm.js (1)
489-500:⚠️ Potential issue | 🟠 MajorUse the correct class name for tooltip wrapper styling.
The
TextAreauses"full-width", which is not defined anywhere in the codebase. Use"bdl-UnifiedShareModal-tooltipWrapper"instead, which is the established class defined inUnifiedShareModal.scssand setswidth: 100%:Suggested fix
<TextArea data-testid="be-emailform-message" label={<FormattedMessage {...messages.messageTitle} />} onChange={this.handleMessageChange} placeholder={intl.formatMessage(commonMessages.messageSelectorPlaceholder)} rows={3} - tooltipWrapperClassName="full-width" + tooltipWrapperClassName="bdl-UnifiedShareModal-tooltipWrapper" value={message} {...messageProps} />
🧹 Nitpick comments (1)
src/features/unified-share-modal/ContactsField.js (1)
28-29: Consider consistent prop naming across components.The prop is named
targetWrapperClassNamehere buttooltipWrapperClassNameinPillSelector,PillSelectorDropdown, andTextArea. While the current implementation works (mapping totooltipWrapperClassNameat line 219), this naming inconsistency may confuse consumers of the API.Consider renaming to
tooltipWrapperClassNamefor consistency with sibling components:♻️ Suggested refactor for naming consistency
type Props = { disabled: boolean, error: string, fieldRef?: Object, getContactAvatarUrl?: (contact: Contact) => string, getContacts: (query: string) => Promise<Array<Contact>>, getPillClassName?: (option: SelectOptionProp) => string, intl: any, label: React.Node, - /** A CSS class for the tooltip's target wrapper element */ - targetWrapperClassName?: string, + /** A CSS class for the tooltip's target wrapper element */ + tooltipWrapperClassName?: string, onContactAdd: Function,showContactAvatars, - targetWrapperClassName, + tooltipWrapperClassName, validateForError, validator, } = this.props;selectorOptions={contacts} - tooltipWrapperClassName={targetWrapperClassName} + tooltipWrapperClassName={tooltipWrapperClassName} validateForError={validateForError}Also applies to: 180-180, 219-219
|
Can we add some tests for the change? Also the snapshots seem to be outdated |
|
|
||
| .bdl-UnifiedShareModal-tooltipWrapper { | ||
| width: 100%; | ||
| } |
There was a problem hiding this comment.
It's not clear to me why tooltip wrapper impacts the input ?
There was a problem hiding this comment.
the latest Tooltip change added display: inline-block; to tooltip wrapper
- provide tooltip class wrapper to force elements occupy full width
8f392a7 to
d76f61a
Compare
No description provided.