-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Negative coh red #2682
Negative coh red #2682
Conversation
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.
I like the idea 👌
} | ||
} | ||
}); | ||
}); |
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.
Could we move this in its own file? Then this is more easily adjustable by stores.
@@ -98,6 +98,8 @@ dl { | |||
.red { color: $color-5 } | |||
.yellow { color: $color-6 } | |||
|
|||
.negative { color: $color-5 !important } |
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.
Could we use a meaningful color variable (we have $red
) here?
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.
Do we really need the !important
here? Especially with this generic class this could cause headaches. Would a stronger selector like
input[type="number"].negative
work as well?
document.querySelector('body').addEventListener('input', function(e){ | ||
var el = e.target; | ||
var isInputNumber = el instanceof HTMLInputElement && el.type == 'number'; | ||
if(isInputNumber){ |
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.
Could we please the human eye by adding some spacing here?
var el = e.target; | ||
var isInputNumber = el instanceof HTMLInputElement && el.type == 'number'; | ||
if(isInputNumber){ | ||
if(el.value < 0){ |
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.
Ditto?
Note to self. Enable javascript syntax linting on HoundCI |
This commit aims to smooth out admin user interactions when dealing with number by providing visual feedback of negative numbers Having negative amounts of items is unintuitive, though necessary for backordering. This commit calls out when a number is negative by highlighting it in red.
Requested changes made, thanks! |
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.
Thanks
This PR aims to improve the user experience of the admin, specifically with the goal of alleviating the #2530. Since we do not know if generally admins prefer to get setting the count on hand (as they do now), or would rather mostly be adjusting it instead, this PR aims instead to provide the admin better visual feedback such that they are more aware when the number they are dealing with is negative or not.