Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(component): DLT-2121 move all styling to dialtone-css from vue #557

Open
wants to merge 15 commits into
base: staging
Choose a base branch
from

Conversation

braddialpad
Copy link
Contributor

@braddialpad braddialpad commented Oct 31, 2024

fix(component): DLT-2121 move all styling to dialtone-css from vue

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-2121

📖 Description

Moved all remaining CSS styling from core components into dialtone-css.

💡 Context

We want all our styling in one place

📝 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.

For all CSS changes:

  • I have used design tokens whenever possible.

🔮 Next Steps

Do the same thing on recipes

BREAKING CHANGE CLASSNAME LIST

We will likely have to do a small find/replace migration to fix code overriding these classes. d-item-layout and d-list-item are the highest risk. I would say usage of any of the others should be rare.

Note if this will also include all sub elements such as d-list-item--focusable

dt-item-layout -> d-item-layout (and all sub elements)
dt-list-item -> d-list-item (and all sub elements)
enter-active -> d-collapsible__enter-active
leave-active -> d-collapsible__leave-active
dt-empty-list-item -> d-combobox__empty-list-item
dt-description-list -> d-description-list
dt-list-separator -> d-list-item-separator
mention-suggestion-name -> d-mention-suggestion__name
dt-suggestion-list__container -> d-suggestion-list__container
dt-rich-text-editor -> d-rich-text-editor
skeleton-placeholder -> d-skeleton-placeholder

@braddialpad braddialpad added the visual-test-ready Add this tag when the PR is ready for visual test, to trigger GHA visual tests label Oct 31, 2024
Copy link
Contributor

@francisrupert francisrupert left a comment

Choose a reason for hiding this comment

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

Awesome. A few basic comments. I haven't gone through all them yet, though first one I noticed still with dt- classes is Item Layout (with List Item).

.skeleton-placeholder {
--placeholder-from-color: hsla(var(--dt-color-black-900-hsl) / 0.2);
--placeholder-to-color: hsla(var(--dt-color-black-900-hsl) / 0.1);

display: flex;
width: 100%;
background: var(--placeholder-from-color);
background: var(--placeholder-from-color, var(--dt-color-black-300));
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's make the fallback color var(--dt-color-surface-bold-opaque). Same for the others below.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What about the --placeholder-to-color? currently falls back to var(--dt-color-black-100)

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah yes, use var(--dt-color-surface-moderate-opaque). That doesn't map to the current Black 100, but it's a less jarring animating shift.

Also, remove the...

style="
  ...
  --placeholder-from-color: ...;
  --placeholder-to-color: ...;
  ...
"

...from the ## Prefabricated examples.

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe belay that, look at this other comment first.

But still do the removal of style="..." comment above.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Alright, should be good.

@braddialpad
Copy link
Contributor Author

Looking into percy issues, a couple small problems found there.

@braddialpad
Copy link
Contributor Author

Alright, all remaining issues should be fixed. Also added a blog post for the breaking change since it could affect anyone overriding component classes.

Copy link

github-actions bot commented Nov 2, 2024

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
visual-test-ready Add this tag when the PR is ready for visual test, to trigger GHA visual tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants