User tests: Successful: Unsuccessful:
Pull Request for Issue #43159.
Alternative to PR #43161
Removes the extra border on the sidebar that is producing a pixel shift between light and dark mode.
Prevent pixel shift and still keep the boundary by removing the border and setting the already existing variable --atum-box-shadow in dark mode for the element.
As this is an SCSS change you you will need to test with a the pre-built package from the download section of this PR, or run npm ci
after applying the PR.
Please select:
Documentation link for docs.joomla.org:
No documentation changes for docs.joomla.org needed
Pull Request link for manual.joomla.org:
No documentation changes for manual.joomla.org needed
Status | New | ⇒ | Pending |
Category | ⇒ | Repository NPM Change |
not a fan of this approach to changing a variable
Sorry @brianteeman I just checked and see I missed to check in my review last week.
I can close here if you would reopen your PR.
Labels |
Added:
NPM Resource Changed
PR-5.1-dev
|
not a fan of this approach to changing a variable
I'm afraid I don't quite understand. If we don't use the css variables we declared for this purpose then we could actually do without it completely and remove it.
I dont see anywhere else with this is done. everywhere else it is similar to
box-shadow: var(--atum-box-shadow);
But the idea of having a css variable instead of declaring the values directly, is that I can change the value of the variable (globally or only in a specific context) without having to reassign it to the element.
https://developer.mozilla.org/en-US/docs/Web/CSS/Using_CSS_custom_properties
yes - exactly my point. The way you have done it makes it no different to declaring the values directly.
I think I struggle a little bit with your explanation
Here we have declared the class sidebar-wrapper should use the variable, that has nothing to do with my PR
And at the moment here we ignore the fact that we actually have a variable for it and do not change the value of the variable in dark mode but set the box-shadow explicitly to none.
An alternative would be
@if $enable-dark-mode {
@include color-mode(dark) {
.sidebar-wrapper {
...
box-shadow: 1px 0 0 0 #ffffff0d;
....
}
But I don't see the benefit.
For the sidebar-wrapper I see no need to have the box-shadow completely around the wrapper, only on the left side to keep contrast for the end of the sidebar, so I have to modify the value to use the already declared variable for sidebar-wrapper.
Is there something I am missing or I can't see?
never mind - i am not good at explaining this
If you define it as a variable then it is easy to customise. What you have done is no different to box-shadow: 1px 0 0 0 #ffffff0d;
I would have created a new variable --atum-sidebar-shadow
in _variables.scss
and then used box-shadow: var(--atum-sidebar-shadow)
plus you would then be able to correctly have a shadow in an rtl template where the value of the variable would be different.
_variables.scss
--atum-sidebar-shadow: box-shadow: 1px 0 0 0 #ffffff0d;
_variables_rtl.scss
--atum-sidebar-shadow: box-shadow: 0 0 0 1px #ffffff0d;
plus you would then be able to correctly have a shadow in an rtl template where the value of the variable would be different.
_variables.scss --atum-sidebar-shadow: box-shadow: 1px 0 0 0 #ffffff0d;
_variables_rtl.scss --atum-sidebar-shadow: box-shadow: 0 0 0 1px #ffffff0d;
I did not understand why there should be a different value for rtl.
Based on https://developer.mozilla.org/en-US/docs/Web/CSS/box-shadow#syntax
box-shadow: <offset-x> <offset-y> <blur-radius> <spread-radius> <color>
Reason why I propose LadySolveig#2 for cpanels:
box-shadow: 0 0 0 1px rgba(255, 255, 255, .1);
I may have the syntax wrong - I was doing it from memory
However if you are setting a box shadow to appear on the right hand side of the sidebar then obviously on an RTL display you would need to set the box shadow to appear on the left hand side of the sidebar
This pull request has been automatically rebased to 5.2-dev.
Title |
|
;) you could have just asked for changes in my pr. that is the normal way to work ikn a collaborative environment