Skip to content

ENG-2732: Change description span to div#7502

Merged
tvandort merged 5 commits intomainfrom
ENG-2732
Mar 12, 2026
Merged

ENG-2732: Change description span to div#7502
tvandort merged 5 commits intomainfrom
ENG-2732

Conversation

@tvandort
Copy link
Contributor

@tvandort tvandort commented Feb 26, 2026

Ticket ENG-2732

Description Of Changes

Change experience description to use a div instead of a span because span doesn't support all tags as descendants. Notably a client reporting that p tags were being mangled.

Code Changes

  • Change p to div

Steps to Confirm

  1. Allow HTML descriptions
  2. Configure an experience description that contains a p tag
  3. Load that experience on a page somewhere and ensure that p tag isn't mangled

Pre-Merge Checklist

  • Issue requirements met
  • All CI pipelines succeeded
  • CHANGELOG.md updated
    • Add a db-migration This indicates that a change includes a database migration label to the entry if your change includes a DB migration
    • Add a high-risk This issue suggests changes that have a high-probability of breaking existing code label to the entry if your change includes a high-risk change (i.e. potential for performance impact or unexpected regression) that should be flagged
    • Updates unreleased work already in Changelog, no new entry necessary
  • UX feedback:
    • All UX related changes have been reviewed by a designer
    • No UX review needed
  • Followup issues:
    • Followup issues created
    • No followup issues
  • Database migrations:
    • Ensure that your downrev is up to date with the latest revision on main
    • Ensure that your downgrade() migration is correct and works
      • If a downgrade migration is not possible for this change, please call this out in the PR description!
    • No migrations
  • Documentation:
    • Documentation complete, PR opened in fidesdocs
    • Documentation issue created in fidesdocs
    • If there are any new client scopes created as part of the pull request, remember to update public-facing documentation that references our scope registry
    • No documentation updates required

Summary by CodeRabbit

  • Bug Fixes
    • Expanded HTML tag support in experience descriptions by updating the container element type, enabling a broader range of HTML elements to render correctly while maintaining existing functionality.

@vercel
Copy link
Contributor

vercel bot commented Feb 26, 2026

The latest updates on your projects. Learn more about Vercel for GitHub.

2 Skipped Deployments
Project Deployment Actions Updated (UTC)
fides-plus-nightly Ignored Ignored Preview Mar 12, 2026 3:29pm
fides-privacy-center Ignored Ignored Mar 12, 2026 3:29pm

Request Review

@tvandort
Copy link
Contributor Author

I am admittedly a little worried about this change. The good thing is that span doesn't have default styling or any semantics, much like div. There is a difference in which element is a block or not by default which might get me into trouble here. I'd appreciate some help thinking what could go wrong as a result of this change.

@tvandort tvandort requested a review from gilluminate March 11, 2026 14:34
@tvandort tvandort marked this pull request as ready for review March 11, 2026 14:34
@tvandort tvandort requested a review from a team as a code owner March 11, 2026 14:34
@coderabbitai
Copy link

coderabbitai bot commented Mar 11, 2026

📝 Walkthrough

Walkthrough

Replace span with div element in ExperienceDescription component when HTML descriptions are allowed. Document this DOM structure change in a new changelog entry for ENG-2732.

Changes

Cohort / File(s) Summary
Changelog documentation
changelog/ENG-2732-fix-experience-description-html.yaml
New changelog entry documenting the span-to-div replacement in ExperienceDescription, categorized as a Changed type modification.
Component HTML element
clients/fides-js/src/components/ExperienceDescription.tsx
Replace span wrapper with div in renderString method when allowHTMLDescription is true; no logic or control flow changes.

Estimated code review effort

🎯 1 (Trivial) | ⏱️ ~3 minutes

Poem

A span becomes a div so wide, 🐰
To hold descriptions side by side,
In fides-js, a gentle shift,
HTML tags now get a lift!

🚥 Pre-merge checks | ✅ 3
✅ Passed checks (3 passed)
Check name Status Explanation
Title check ✅ Passed The title accurately summarizes the main change in the PR: replacing a span element with a div in the experience description component.
Description check ✅ Passed The description follows the required template with all major sections populated: ticket reference, description of changes, code changes, confirmation steps, and pre-merge checklist.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
  • 📝 Generate docstrings (stacked PR)
  • 📝 Generate docstrings (commit on current branch)
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch ENG-2732

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

🧹 Nitpick comments (1)
changelog/ENG-2732-fix-experience-description-html.yaml (1)

3-3: Remove stale TODO comment.

The PR number 7502 is already correct. The TODO comment should be removed before merging.

✏️ Suggested fix
-pr: 7502 # TODO: Update with actual PR number once created
+pr: 7502
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@changelog/ENG-2732-fix-experience-description-html.yaml` at line 3, Remove
the stale inline TODO by editing the YAML entry that reads "pr: 7502 # TODO:
Update with actual PR number once created" and delete the trailing comment so
the line is simply "pr: 7502" (update only that comment text; leave the PR value
intact).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In `@changelog/ENG-2732-fix-experience-description-html.yaml`:
- Line 3: Remove the stale inline TODO by editing the YAML entry that reads "pr:
7502 # TODO: Update with actual PR number once created" and delete the trailing
comment so the line is simply "pr: 7502" (update only that comment text; leave
the PR value intact).

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 245362f1-7654-4646-b305-70351e23ac40

📥 Commits

Reviewing files that changed from the base of the PR and between 9db995b and b528846.

📒 Files selected for processing (2)
  • changelog/ENG-2732-fix-experience-description-html.yaml
  • clients/fides-js/src/components/ExperienceDescription.tsx

@greptile-apps
Copy link
Contributor

greptile-apps bot commented Mar 11, 2026

Greptile Summary

This PR fixes a browser DOM-mangling bug where <p> tags (and other block-level elements) inside experience descriptions were being restructured by the browser because they were children of an inline <span>. The fix swaps <span> for <div> in renderString and applies display: contents to the wrapper so that the existing VENDOR_COUNT_LINK inline button layout is preserved.

  • ExperienceDescription.tsx: renderString now returns a <div className="fides-experience-description-part"> instead of a <span>, making block-level HTML children (e.g. <p>) spec-compliant.
  • fides.css: The new .fides-experience-description-part { display: contents } rule makes the wrapper div invisible to the layout engine, so its children (text fragments and the vendor-count button) continue to flow inline as before — this addresses the previously raised concern about the VENDOR_COUNT_LINK layout regression.
  • changelog/ENG-2732-fix-experience-description-html.yaml: Correctly records the change with the right PR number and no stale TODO comment.

Confidence Score: 5/5

  • This PR is safe to merge — the change is minimal, targeted, and the previously raised concerns around layout regression and display: inline have been resolved with display: contents.
  • The diff is three small files. The core fix (spandiv) is straightforward and correct. The display: contents CSS rule elegantly preserves the inline flow for the VENDOR_COUNT_LINK case without introducing block-in-inline rendering issues. All previous review threads have been addressed.
  • No files require special attention.

Important Files Changed

Filename Overview
clients/fides-js/src/components/ExperienceDescription.tsx Changed renderString to return a <div> (with className="fides-experience-description-part") instead of a <span> so block-level HTML children like <p> are valid. The layout side-effect is handled in fides.css.
clients/fides-js/src/components/fides.css Added .fides-experience-description-part { display: contents; } so the wrapper div is invisible to the layout engine, preserving the inline text flow needed for the VENDOR_COUNT_LINK button.
changelog/ENG-2732-fix-experience-description-html.yaml New changelog entry for the fix; PR number is correct and no stale TODO comment remains.

Last reviewed commit: 7070b4a

@tvandort tvandort force-pushed the ENG-2732 branch 2 times, most recently from b528846 to c17731b Compare March 11, 2026 17:57
@tvandort
Copy link
Contributor Author

@greptileai rereview

@tvandort
Copy link
Contributor Author

@greptile rereview

Copy link
Contributor

@gilluminate gilluminate left a comment

Choose a reason for hiding this comment

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

love it. ship it

@tvandort tvandort added this pull request to the merge queue Mar 12, 2026
Merged via the queue into main with commit 73420db Mar 12, 2026
42 checks passed
@tvandort tvandort deleted the ENG-2732 branch March 12, 2026 18:14
@greptile-apps greptile-apps bot mentioned this pull request Mar 12, 2026
18 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants