NPM Resource Changed PR-5.1-dev Pending

User tests: Successful: Unsuccessful:

avatar LadySolveig
LadySolveig
3 Apr 2024

Pull Request for Issue #43159.

Summary of Changes

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.

Testing Instructions

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.

Actual result BEFORE applying this Pull Request

Expected result AFTER applying this Pull Request

Link to documentations

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

avatar LadySolveig LadySolveig - open - 3 Apr 2024
avatar LadySolveig LadySolveig - change - 3 Apr 2024
Status New Pending
avatar joomla-cms-bot joomla-cms-bot - change - 3 Apr 2024
Category Repository NPM Change
avatar brianteeman
brianteeman - comment - 3 Apr 2024

;) you could have just asked for changes in my pr. that is the normal way to work ikn a collaborative environment

avatar brianteeman
brianteeman - comment - 3 Apr 2024

not a fan of this approach to changing a variable

avatar LadySolveig
LadySolveig - comment - 3 Apr 2024

Sorry @brianteeman I just checked and see I missed to check in my review last week.
grafik
I can close here if you would reopen your PR.

avatar LadySolveig LadySolveig - change - 3 Apr 2024
Labels Added: NPM Resource Changed PR-5.1-dev
avatar LadySolveig
LadySolveig - comment - 3 Apr 2024

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.

avatar brianteeman
brianteeman - comment - 3 Apr 2024

I dont see anywhere else with this is done. everywhere else it is similar to

box-shadow: var(--atum-box-shadow);

avatar LadySolveig
LadySolveig - comment - 3 Apr 2024

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

avatar brianteeman
brianteeman - comment - 3 Apr 2024

yes - exactly my point. The way you have done it makes it no different to declaring the values directly.

avatar LadySolveig
LadySolveig - comment - 3 Apr 2024

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?

avatar brianteeman
brianteeman - comment - 3 Apr 2024

never mind - i am not good at explaining this

avatar brianteeman
brianteeman - comment - 3 Apr 2024

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)

avatar brianteeman
brianteeman - comment - 3 Apr 2024

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;

avatar Razzo1987
Razzo1987 - comment - 3 Apr 2024

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);

avatar brianteeman
brianteeman - comment - 3 Apr 2024

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

Add a Comment

Login with GitHub to post a comment