Skip to content

Conversation

@humza1400
Copy link

@humza1400 humza1400 commented Mar 14, 2025

User description

✨ Summary

This PR fixes two issues in the Tipseen component:

  1. Replaces the <Text> element’s element="p" with element="div" to resolve invalid HTML structure when rendering nested <div>s inside <p>.
  2. Fixes improper usage of id and ref props on React.Fragment by conditionally rendering a <div> wrapper when necessary.

✅ Changes

  • element prop in Text changed from "p" to "div" to comply with HTML standards.
  • Conditionally render a <div> instead of React.Fragment when id or ref is passed, avoiding React warnings.
  • No changes in component behavior or appearance (verified in Storybook).

🧪 How I tested it

  • Ran yarn storybook and verified no console warnings in the Tipseen stories.
  • Confirmed component behavior is consistent.
  • Ran yarn test and yarn lint with no issues related to my changes.

Error Messages Before Fix

image
image


PR Type

Bug fix


Description

  • Fix invalid HTML structure by changing Text element from "p" to "div"

  • Resolve React Fragment ref/id prop warnings with conditional wrapper

  • Add ellipsis={false} prop to Text component for consistency


Diagram Walkthrough

flowchart LR
  A["Text element='p'"] --> B["Text element='div'"]
  C["Fragment with ref/id"] --> D["Conditional div wrapper"]
  B --> E["Valid HTML structure"]
  D --> E
Loading

File Walkthrough

Relevant files
Bug fix
Tipseen.tsx
Fix HTML structure and Fragment prop handling                       

packages/core/src/components/Tipseen/Tipseen.tsx

  • Changed Text element prop from "p" to "div" for valid HTML
  • Added conditional wrapper logic to handle ref/id props properly
  • Extracted wrapper props to avoid React Fragment warnings
  • Added ellipsis={false} prop to Text component
+4/-2     

@humza1400
Copy link
Author

humza1400 commented Apr 9, 2025

Bump

@talkor

@qodo-merge-for-open-source
Copy link
Contributor

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪
🧪 No relevant tests
🔒 No security concerns identified
⚡ Recommended focus areas for review

Possible Issue

When using the Fragment branch (no ref/id), the wrapper no longer receives a data-testid. If tests rely on finding the Tipseen root by test id, they may fail; consider preserving a stable test id on an inner element.

const TipseenWrapper = ref || id ? "div" : Fragment;
const wrapperProps = TipseenWrapper === "div" ? { ref: mergedRef, id, "data-testid": dataTestId || getTestId(ComponentDefaultTestId.TIPSEEN, id) } : {};

const tooltipContent = (
  <div>
    <div className={cx(styles.tipseenHeader)}>
      {hideCloseButton ? null : (
        <IconButton
          hideTooltip
          className={cx(styles.tipseenCloseButton, {
            [styles.dark]: closeButtonTheme === "dark" || closeButtonTheme === "fixed-dark"
          })}
          onClick={onClose}
          size="xs"
          kind="tertiary"
          // @ts-ignore
          color={closeButtonColor}
          ariaLabel={overrideCloseAriaLabel}
          icon={CloseSmall}
        />
      )}
      <TipseenTitle text={title} className={cx(styles.tipseenTitle, titleClassName)} />
    </div>
    <Text color={textColor} type="text2" element="div" ellipsis={false} className={cx(styles.tipseenContent)}>
      <TipseenContext.Provider value={color}>{content}</TipseenContext.Provider>
    </Text>
  </div>
);

return (
  <TipseenWrapper {...wrapperProps}>
    <Tooltip
Accessibility

Changing Text element from p to div can affect semantics. Ensure the tooltip body still has appropriate roles/semantics (e.g., role, aria attributes) and maintains readable structure for screen readers.

<Text color={textColor} type="text2" element="div" ellipsis={false} className={cx(styles.tipseenContent)}>
  <TipseenContext.Provider value={color}>{content}</TipseenContext.Provider>
</Text>

Copy link
Member

@talkor talkor left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you @humza1400 ! And sorry for the long delay
Please just fix the lint issue (Prettier) and I'll merge
Thanks!!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants