Skip to content

refactor(components): DLT-3100 remove rootClass references#1184

Open
Ignacio Ropolo (iropolo) wants to merge 4 commits intonextfrom
remove-root-class-from-components
Open

refactor(components): DLT-3100 remove rootClass references#1184
Ignacio Ropolo (iropolo) wants to merge 4 commits intonextfrom
remove-root-class-from-components

Conversation

@iropolo
Copy link
Copy Markdown
Contributor

@iropolo Ignacio Ropolo (iropolo) commented Apr 7, 2026

Remove rootClass references

🛠️ Type Of Change

These types will increment the version number on release:

  • Refactor

📖 Jira Ticket

This is just the first part of the ticket.

https://dialpad.atlassian.net/browse/DLT-3100

📖 Description

Updated rootClass to be only class on components:

Component Changes
InputMixin Removed rootClass prop - classes now passed via $attrs
Input Removed rootClass prop and usage in template
Checkbox Removed rootClass usage from template (was from mixin)
Radio Removed rootClass usage from template (was from mixin)
SelectMenu Removed rootClass, added inputClass for the select element
Toggle Removed wrapperClass prop, uses class via $attrs
Card Removed containerClass prop, added inheritAttrs: false, uses class via $attrs
BreadcrumbItem Removed rootClass prop
SplitButton Removed rootClass prop, uses class via $attrs
FeedItemPill Removed wrapperClass prop, added inheritAttrs: false, uses class via $attrs
Combinator Removed unused rootClass/headerClass props

Breaking Changes

  • Any consumer code using rootClass, containerClass, or wrapperClass props will silently fail - the props will be ignored and classes won't be applied
  • No runtime errors will occur, but styling will break
  • This affects: Input, Checkbox, Radio, SelectMenu, Toggle, Card, BreadcrumbItem, SplitButton, FeedItemPill
Component Removed Prop Migration
Input rootClass Use class attribute directly
Checkbox rootClass Use class attribute directly
Radio rootClass Use class attribute directly
SelectMenu rootClass Use class attribute directly
Toggle wrapperClass Use class attribute directly
Card containerClass Use class attribute directly
BreadcrumbItem rootClass Use class attribute directly
SplitButton rootClass Use class attribute directly
FeedItemPill wrapperClass Use class attribute directly

New Props (not breaking, but worth noting)

Component New Prop Purpose
SelectMenu inputClass Pass classes to the  element

Before:

<dt-input root-class="my-custom-class" />
<dt-toggle wrapper-class="my-toggle-class" />
<dt-card container-class="my-card-class" />

After:

<dt-input class="my-custom-class" />
<dt-toggle class="my-toggle-class" />
<dt-card class="my-card-class" />

Side note: I also removed addClassStyleAttrs($attrs) since seems like it is for Vue2 compat mode which is not needed anymore.
It is still present on some recipes, if this get approves I will remove it from there too.

There is still inputWrapperClass on input.vue and wrapperClass on ListItem
They are for a nested wrapper element, not the root but ... stil confusing..
on input.vue we still have inputClass and inputWrapperClass. At least we don't have rootClass anymore. My goodness

@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Apr 7, 2026

Please add either the visual-test-ready or no-visual-test label to this PR depending on whether you want to run visual tests or not.
It is recommended to run visual tests if your PR changes any UI. ‼️

@iropolo Ignacio Ropolo (iropolo) added the no-visual-test Add this tag when the PR does not need visual testing label Apr 7, 2026
@iropolo Ignacio Ropolo (iropolo) marked this pull request as ready for review April 7, 2026 21:27
Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector bot left a comment

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: ca84a76137

ℹ️ About Codex in GitHub

Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "Codex (@codex) review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "Codex (@codex) address that feedback".

Comment on lines +4 to +5
:class="['d-input__root', { 'd-input--hidden': hidden }, $attrs.class]"
:style="$attrs.style"
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P1 Badge Migrate remaining root-class call sites

This change makes root styling depend on $attrs.class and removes rootClass, but there are still in-repo callers using root-class (for example packages/dialtone-vue/recipes/conversation_view/editor/editor.vue and packages/combinator/src/components/option_bar/*.vue). Because this component has inheritAttrs: false, those legacy attrs are no longer interpreted as root classes, so expected wrapper styling is dropped and layouts regress in those screens. Please either keep a temporary rootClass compatibility path or update those call sites in the same PR.

Useful? React with 👍 / 👎.

compatConfig: { MODE: 3 },
name: 'DtCard',

inheritAttrs: false,
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Preserve passthrough attrs on DtCard root

Adding inheritAttrs: false here drops all non-prop attributes unless they are manually rebound, and the template currently only reapplies class/style. That means consumer attributes like id, data-*, and aria-* no longer reach the root card element, which can break selectors and accessibility hooks. Forwarding the remaining $attrs to the root (while handling class/style explicitly) avoids this regression.

Useful? React with 👍 / 👎.

@francisrupert
Copy link
Copy Markdown
Contributor

Francis Rupert (francisrupert) commented Apr 8, 2026

Breaking Changes

  • Any consumer code using rootClass, containerClass, or wrapperClass props will silently fail - the props will be ignored and classes won't be applied
  • No runtime errors will occur, but styling will break

Ignacio Ropolo (@iropolo) I took the liberty of creating DLT-3281 as a followup action item within DLT-2980, including a speculative planning document.

This is a highly repeatable pattern that should be fairly straight-forward to provide a codemod-like migration script, and even lint with autofix.

When I've created the migration scripts on others within next, using the doc site itself as a test goes a long way. For example, DtToggle's current use of wrapper-class result in broken Combinator.

image

@francisrupert
Copy link
Copy Markdown
Contributor

This is just the first part of the ticket. https://dialpad.atlassian.net/browse/DLT-3100

Then should DLT-3100 be broken out into multiple stories? Make sense to have one that is solely for this rootClass/containerClass/wrapperClass update, and follow it up for the others.

@francisrupert
Copy link
Copy Markdown
Contributor

Not sure fix is the right prefix. That implies it was a bug, so more of a refactor?

@francisrupert
Copy link
Copy Markdown
Contributor

I'm sure you're treating it this way anyway, but would it be prudent to connect https://dialpad.atlassian.net/browse/DLT-3096 as blocked by this one, https://dialpad.atlassian.net/browse/DLT-3100?

@francisrupert
Copy link
Copy Markdown
Contributor

Francis Rupert (francisrupert) commented Apr 8, 2026

Be sure to test them all out visually. Just my first test on a few shows it applying to both the root and the internal part, e.g. <input>.

Input
image

Checkbox
image

Radio
image

I won't show them all, but you get the idea.

@francisrupert
Copy link
Copy Markdown
Contributor

Just my first test on a few shows it applying to both the root and the internal part, e.g. <input>.

Claude's reaction:

There's the bug. removeClassStyleAttrs only strips class/style when configVue2StyleClassAttrs is truthy (line 255). When that flag is falsy, it returns attrs unchanged — meaning the inner <input> gets v-bind="$attrs" including class and style.

This is a Vue 2 compat guard. In Vue 2, $attrs didn't include class/style, so the filter was unnecessary. But with inheritAttrs: false in Vue 3 mode, $attrs always includes class/style, and the guard skips the filter — so both root and inner elements get the class.

The fix would be: since this PR removes rootClass and manually applies $attrs.class on the root, removeClassStyleAttrs on the inner element needs to always strip class/style, not conditionally. Either remove the configVue2StyleClassAttrs guard, or bypass it in the components that now use inheritAttrs: false with manual $attrs.class.

@iropolo
Copy link
Copy Markdown
Contributor Author

Ignacio Ropolo (iropolo) commented Apr 8, 2026

Not sure fix is the right prefix. That implies it was a bug, so more of a refactor?

I can change it for refactor, it was the old title before I split the task.

Then should DLT-3100 be broken out into multiple stories? Make sense to have one that is solely for this rootClass/containerClass/wrapperClass update, and follow it up for the others.

Sure, will be the way to go.

I'm sure you're treating it this way anyway, but would it be prudent to connect https://dialpad.atlassian.net/browse/DLT-3096 as blocked by this one, https://dialpad.atlassian.net/browse/DLT-3100?

Yeah, no big deal Im on both.

Be sure to test them all out visually.

Im on it, thanks for the images!

@iropolo Ignacio Ropolo (iropolo) changed the title fix(components): DLT-3100 remove rootClass references refactor(components): DLT-3100 remove rootClass references Apr 8, 2026
@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Apr 8, 2026

✔️ Deploy previews ready!
😎 Dialtone documentation preview: https://dialtone.dialpad.com/deploy-previews/pr-1184/
😎 Dialtone-vue preview: https://dialtone.dialpad.com/vue/deploy-previews/pr-1184/

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

Labels

no-visual-test Add this tag when the PR does not need visual testing

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants