Skip to content

fix: DLT-2398 synchronize class attrs on vue 3 components#688

Merged
Brad Paugh (braddialpad) merged 6 commits intostagingfrom
migrate-vue3-class-attrs
Apr 3, 2025
Merged

fix: DLT-2398 synchronize class attrs on vue 3 components#688
Brad Paugh (braddialpad) merged 6 commits intostagingfrom
migrate-vue3-class-attrs

Conversation

@braddialpad
Copy link
Copy Markdown
Contributor

@braddialpad Brad Paugh (braddialpad) commented Apr 1, 2025

fix: synchronize class attrs on vue 3 components

Obligatory GIF (super important!)

Obligatory GIF

🛠️ Type Of Change

These types will increment the version number on release:

  • Fix

📖 Jira Ticket

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

📖 Description

Migrated our class attributes to match the way it works in vue 2 if config option is enabled.

Vue 3 only.

💡 Context

Due to the breaking change INSTANCE_ATTRS_CLASS_STYLE in vue 3, the class and style attributes are not applied to the same elements they were in vue 2.

Any component that uses inheritAttrs: false will now include class and style attributes in $attrs unlike vue 2 which always applied them to the root element. This means in order to have our components behave the same in vue 2 and vue 3 we need to make a small modification to directly apply the class to the root element in the case of inheritAttrs: true.

This could potentially be a breaking change for applications already using the vue 3 library and applying class attributes to inheritAttrs components so I included this behaviour as a configuration option that you can set on init more for migration purposes.

Already tested on the vue 3 migration branch and it does seem to work.

📝 Checklist

For all PRs:

  • I have ensured no private Dialpad links or info are in the code or pull request description (Dialtone is a public repo!).
  • I have reviewed my changes.
  • I have added all relevant documentation.
  • I have considered the performance impact of my change.

🔮 Next Steps

Apply config option in ubervoice vue 3 migration.

🔗 Sources

https://v3-migration.vuejs.org/breaking-changes/attrs-includes-class-style.html

@braddialpad Brad Paugh (braddialpad) added the no-visual-test Add this tag when the PR does not need visual testing label Apr 1, 2025
@braddialpad Brad Paugh (braddialpad) changed the title fix: synchronize class attrs on vue 3 components fix: DLT-2398 synchronize class attrs on vue 3 components Apr 1, 2025
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

As you're removing the class and style attributes doesn't that mean that you should include on all those components a :style="$attrs.style" attribute? even if it's empty, otherwise, if any of those components have a style, it'd be ignored, right?

@braddialpad
Copy link
Copy Markdown
Contributor Author

Brad Paugh (braddialpad) commented Apr 2, 2025

As you're removing the class and style attributes doesn't that mean that you should include on all those components a :style="$attrs.style" attribute? even if it's empty, otherwise, if any of those components have a style, it'd be ignored, right?

Yeah true, if there was one. I didn't do this because we don't generally use inline styles and in fact consider it a bad practice to do so.

@juliodialpad
Copy link
Copy Markdown
Contributor

As you're removing the class and style attributes doesn't that mean that you should include on all those components a :style="$attrs.style" attribute? even if it's empty, otherwise, if any of those components have a style, it'd be ignored, right?

Yeah true, if there was one. I didn't do this because we don't generally use inline styles and in fact consider it a bad practice to do so.

I understand, but as we don't want to do breaking changes, we should include them as they might cause issues if someone used it on product.

Or at least we should put a comment somewhere saying that we remove style attribute on purpose on those components.

@braddialpad
Copy link
Copy Markdown
Contributor Author

As you're removing the class and style attributes doesn't that mean that you should include on all those components a :style="$attrs.style" attribute? even if it's empty, otherwise, if any of those components have a style, it'd be ignored, right?

Yeah true, if there was one. I didn't do this because we don't generally use inline styles and in fact consider it a bad practice to do so.

I understand, but as we don't want to do breaking changes, we should include them as they might cause issues if someone used it on product.

Or at least we should put a comment somewhere saying that we remove style attribute on purpose on those components.

Yeah doesn't hurt to include them I guess..

@braddialpad
Copy link
Copy Markdown
Contributor Author

Added style as requested.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Looking good, thanks!

:is="clickable ? 'button' : 'div'"
:id="id"
:class="avatarClasses"
:style="$attrs.style"
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

why does this file have inheritAttrs false? If it didn't have that what you are doing would be default behavior (plus some other attrs could be inherited)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

100% agree, I will do this in a separate PR though since i'll have to do it on vue 2 and vue 3.

<div
data-qa="dt-recipe-callbox"
class="d-recipe-callbox"
:class="[$attrs.class, 'd-recipe-callbox']"
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

also would be default behavior without inheritattrs false

Comment thread packages/dialtone-vue3/common/utils/index.js
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think the util function should be used in a computed that splits out the style and class attrs if enabled. As it stands, if you don't opt into emulating the vue2 behavior, the class and style gets added in both places.

@braddialpad
Copy link
Copy Markdown
Contributor Author

Brad Paugh (braddialpad) commented Apr 3, 2025

Made requested fixes, will put the two I mentioned in another PR

@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Apr 3, 2025

✔️ Deploy previews ready!
😎 Dialtone preview: https://dialtone.dialpad.com/deploy-previews/pr-688/
😎 Dialtone-vue 2 preview: https://dialtone.dialpad.com/vue/deploy-previews/pr-688/
😎 Dialtone-vue 3 the preview: https://dialtone.dialpad.com/vue3/deploy-previews/pr-688/

@braddialpad Brad Paugh (braddialpad) merged commit ff72641 into staging Apr 3, 2025
10 checks passed
@braddialpad Brad Paugh (braddialpad) deleted the migrate-vue3-class-attrs branch April 3, 2025 22:05
Julio Ortega (juliodialpad) pushed a commit that referenced this pull request Apr 7, 2025
## [3.173.2](dialtone-vue3/v3.173.1...dialtone-vue3/v3.173.2) (2025-04-03)

### Bug Fixes

* DLT-2398 synchronize class attrs on vue 3 components ([#688](#688)) ([ff72641](ff72641))
* NO-JIRA $el in vue 3 ([#690](#690)) ([0027ba7](0027ba7))
Julio Ortega (juliodialpad) pushed a commit that referenced this pull request Apr 7, 2025
## [9.107.2](dialtone/v9.107.1...dialtone/v9.107.2) (2025-04-03)

### Bug Fixes

* DLT-2398 synchronize class attrs on vue 3 components ([#688](#688)) ([ff72641](ff72641))
* NO-JIRA $el in vue 3 ([#690](#690)) ([0027ba7](0027ba7))
* NO-JIRA dialtone icons android build ([#691](#691)) ([8c10768](8c10768))
* **Tooltip:** DP-129564 add overflow-wrap for tooltip ([#687](#687)) ([b6d8256](b6d8256))
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.

3 participants