[sparkle] - chore: ditch z-index classes

[JulesBelveze]

Description

This PR aims at ditching all existing z-indexes on sparkle components and to add a linting rule to prevent from adding new ones.

As we are not doing anything extremely fancy for now there should be no need to use z-indexes. Also, adding z-indexes conflicts with indexes from Radix.

Risk

Quite important blast radius UX wise

Deploy Plan

• Publish sparkle RC
• Upgrade front
• Publish sparkle

[morganroux]

Test comment Morgan

[dust-agent-dev]

I've reviewed the changes in this PR which focuses on removing z-index usage from Sparkle components and implementing a linting rule to prevent future usage. Here's my detailed review based on our coding rules:

Positive Aspects

1. [GEN1] Consistency Improvement

   • The PR enforces a consistent approach by removing all z-index usage across components
   • Adds an ESLint rule to prevent future inconsistencies
   • This aligns well with "Consistently bad is better than inconsistently good"

2. [GEN2] Simplification

   • Removing z-index management simplifies the component styling approach
   • Makes the codebase easier to understand and maintain
   • Example from BarHeader.tsx:

   // Before
   "s-fixed s-left-0 s-right-0 s-top-0 s-z-30 s-flex..."
   // After
   "s-fixed s-left-0 s-right-0 s-top-0 s-flex..."

3. [REACT1] Props Interface Compliance

   • The modified components maintain their proper Props interfaces
   • Example from Citation.tsx shows proper interface usage:

   interface CitationImageProps {
     className?: string;
     ...
   }

4. [REACT3] Visual States

   • Loading states and visual feedback are preserved despite z-index removal
   • Example from DropzoneOverlay.tsx maintains visual feedback:

   className="s-absolute s-inset-0 s-flex s-h-full s-w-full s-flex-col..."

Suggestions for Improvement

1. Documentation

   • Consider adding a comment in the ESLint configuration explaining why z-indexes are forbidden
   • Current config in .eslintrc.js:

   "no-restricted-syntax": [
     "error",
     {
       selector: "JSXAttribute[name.name='className'] Literal[value=/\\bs-z-/]",
       message: "Usage of s-z- tailwind classes (z-index) is forbidden.",
     },
   ],

   • Could benefit from a more detailed explanation

2. Testing

   • [TEST1] Consider adding functional tests to verify that the UI layering still works correctly without z-index
   • Important for components like Modal, Dialog, and Popover that rely on proper stacking

3. Migration Strategy

   • Consider documenting the recommended approach for handling layering without z-index
   • This would help future contributors understand how to handle cases where they might be tempted to use z-index

Potential Risks

1. Component Layering

   • Removing z-index could affect how components stack in complex UI scenarios
   • Particularly important for components like:

   // Dialog.tsx
   "s-fixed s-inset-0 s-bg-muted-foreground/75..."
   // Notification.tsx
   "s-pointer-events-none s-fixed s-bottom-0 s-right-0..."

2. Radix UI Integration

   • The PR mentions conflicts with Radix indexes
   • Make sure all Radix components still function correctly after these changes

Conclusion

This PR aligns well with our coding rules, particularly [GEN1] and [GEN2], by enforcing a consistent and simpler approach to component layering. The changes are well-structured and the addition of the ESLint rule will help maintain this standard.

Recommendation: Approve with the suggestion to add more documentation about the reasoning behind the z-index removal and guidelines for handling component layering without z-index.