fix: static dialog nobackdrop position fix [ci-visual] #2877
Conversation
|
@InnaAtanasova I'll add breaking changes once PR is approved |
|
✔️ Deploy Preview for fundamental-styles ready! 🔨 Explore the source changes: 75c6ef0 🔍 Inspect the deploy log: https://app.netlify.com/sites/fundamental-styles/deploys/6182b604784bd20008fb2f52 😎 Browse the preview: https://deploy-preview-2877--fundamental-styles.netlify.app |
src/styles/dialog.scss
Outdated
| } | ||
| } | ||
|
|
||
| &__no-backdrop:not(.#{$block}__targeted) { |
There was a problem hiding this comment.
Is it a modifier class or an element? Because from the naming it looks like you are introducing a new html element with the class .fd-dialog__no-backdrop. If it's a modifier class, added on the same level as .fd-dialog, then you should have a different naming, like .fd-dialog--no-backdrop.
src/styles/dialog.scss
Outdated
| } | ||
|
|
||
| &__no-backdrop:not(.#{$block}__targeted) { | ||
| position: fixed; |
There was a problem hiding this comment.
why the position is fixed and not absolute?
There was a problem hiding this comment.
If it's absolute you can use the mixin we already have for this:
@mixin fd-absolute-center {
position: absolute;
left: 50%;
top: 50%;
transform: translate(-50%, -50%);
}
There was a problem hiding this comment.
Well, I decided to keep fixed because it centers the element in the view, not in the parent. Although in fundamental-ngx both fixed and absolute will work (because parent fd-dialog-container is attached directly to body), it feels reasonable to provide unified solution, that works well in all scenarios
There was a problem hiding this comment.
Awesome :) In this case please adjust the fd-absolute-center mixin to accept as parameter the position and reuse it in your code.
You can do something like:
@mixin fd-absolute-center($position: absolute) {
position: $position;
left: 50%;
top: 50%;
transform: translate(-50%, -50%);
}
There was a problem hiding this comment.
You can also rename the mixin to fd-position-center or something similar to make more sense
There was a problem hiding this comment.
sure, done
Usually you add the Breaking changes before opening the PR as the reviewer's job is to make sure they are added. If you get the approval nothing stops you from merging the PR and there's no guarantee that you will add the Breaking changes. |
dmitry-stepanenko
force-pushed
the
ds/5286-dialog-no-backdrop
branch
from
a99ac16 to
e2f6753
Compare
Updated the wiki |
dmitry-stepanenko
force-pushed
the
ds/5286-dialog-no-backdrop
branch
2 times, most recently
from
ba178f9 to
f545a75
Compare
|
@dmitry-stepanenko you need to trigger the visual checks |
dmitry-stepanenko
force-pushed
the
ds/5286-dialog-no-backdrop
branch
from
f545a75 to
66b535a
Compare
dmitry-stepanenko
force-pushed
the
ds/5286-dialog-no-backdrop
branch
from
66b535a to
75c6ef0
Compare
|
🚀 |
Related Issue
Is being done as part of the fix for SAP/fundamental-ngx#5286
Description
Added styles for dialog without backdrop
Screenshots
Before:
After:
Please check whether the PR fulfills the following requirements
remfd-*class is used in the filefd-rtl,fd-ellipsis,fd-flex,fd-selected,fd-focus, ect.)fd-reset()mixin is applied to all elementsnormalizeoptionunnormalizeoption[ci visual]so it can trigger chromatic visual regression (e.g.test: run chromatic visual regression [ci visual])