-
Notifications
You must be signed in to change notification settings - Fork 4
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
refactor: DLT-2122 move css out of recipes #577
Conversation
Signed-off-by: Julio Ortega <julio.ortega@dialpad.com>
Signed-off-by: Julio Ortega <julio.ortega@dialpad.com>
Signed-off-by: Julio Ortega <julio.ortega@dialpad.com>
I will take a look to this today. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wow, that's a lotta work, but quite necessary. I didn't go through each line, but here are a few themes I see repeated throughout:
1. There's quite a bit of nesting going on, resulting in over-specific CSS selectors. Unless there's a really good reason, keep them as flat as possible, e.g.
.dt-recipe-attachment-carousel__progress-bar {
...
.dt-recipe-attachment-carousel__progress-bar__circle {
...
}
}
...can probably be...
.dt-recipe-attachment-carousel__progress-bar {
...
}
.dt-recipe-attachment-carousel__progress-bar__circle {
...
}
...or perhaps...
.dt-recipe-attachment-carousel {
&__progress-bar {
...
}
&__progress-bar__circle {
...
}
}
2. The above example also has some double-BEM'ing going on wit __
. To take the above as an example, perhaps...
.dt-recipe-attachment-carousel {
&__progress-bar {
...
}
&__progress-bar-circle {
...
}
}
It's a case by case judgement call, and naming... things is fun... and hard.
3. Should data-qa
values also match BEM convention? I don't have an opinion on that. e.g.
class="dt-recipe-feed-item-row__left-time"
data-qa="dt-recipe-feed-item-row--left-time"
@@ -250,7 +250,7 @@ A node in IVR like branch that has a condition instead of dtmf connector. | |||
:node-type="branch" | |||
> | |||
<template #connector> | |||
<div class="ivr-connector d-px8 d-h24 d-bar-pill d-mbn12 d-fc-neutral-white d-fs-100">Add branch</div> | |||
<div class="dt-recipe-ivr-node__connector d-px8 d-h24 d-bar-pill d-mbn12 d-fc-neutral-white d-fs-100">Add branch</div> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should these CSS utilities values be folded into .dt-recipe-ivr-node__connector
? There are a few others like that throughout this diff.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah I think so for sure
Should be ok, the remaining percy error is an icon update (expected). edit: didn't see Fracis comment. My comment was regarding fixing PR percy errors. |
Seems like there's an additional border problem on the latest build |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since this is such a big change with high potential for conflicts, I'd rather get this into the main branch and make francis' requested changes in a separate PR. The only thing we want to change before merging here are any further visual regressions.
Merged in staging, going to see if it fixes the final percy error |
✔️ Deploy previews ready! |
Alright looks good! I'm going to merge this and create a separate "clean up" PR |
## [1.0.1](combinator/v1.0.0...combinator/v1.0.1) (2024-12-10) ### Code Refactoring * DLT-2122 move css out of recipes ([#577](#577)) ([75d5a0b](75d5a0b))
## [8.46.3](dialtone-css/v8.46.2...dialtone-css/v8.46.3) (2024-12-10) ### Code Refactoring * DLT-2122 move css out of recipes ([#577](#577)) ([75d5a0b](75d5a0b))
Move CSS within vue recipes to dialtone-css
Obligatory GIF (super important!)
🛠️ Type Of Change
These types will increment the version number on release:
📖 Jira Ticket
https://dialpad.atlassian.net/browse/DLT-2122
📖 Description
💡 Context
We're moving all CSS defined in dialtone vue to dialtone-css.
📝 Checklist
For all PRs:
For all Vue changes:
./scripts/dialtone-vue-sync.sh
script. Read docs here: Dialtone Vue Sync ScriptFor all CSS changes:
🔮 Next Steps
Visually test and review all the recipes to check that everything's still working as expected.