Format availability for the new style #159
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ShaiMagal
previously approved these changes
Dec 5, 2024
kpodemski
previously requested changes
Dec 6, 2024
There was a problem hiding this comment.
I don't see a hardcoded color in this file except for your changes. Shouldn't we use existing variables or introduce a new one?
I understand that you see it as a "dead theme", but it will be a default theme for PrestaShop 9 anyway, so... let's keep things clear.
|
@kpodemski Dude, please, check the code. You use this: <div class="alert alert-primary" role="alert">
<i class="material-icons">person</i> This is a primary alert—check it out!
</div>
<div class="alert alert-secondary" role="alert">
<i class="material-icons">person</i> This is a secondary alert—check it out!
</div>
<div class="alert alert-success" role="alert">
<i class="material-icons">person</i> This is a success alert—check it out!
</div>
<div class="alert alert-danger" role="alert">
<i class="material-icons">person</i> This is a danger alert—check it out!
</div>
<div class="alert alert-warning" role="alert">
<i class="material-icons">person</i> This is a warning alert—check it out!
</div>
<div class="alert alert-info" role="alert">
<i class="material-icons">person</i> This is a info alert—check it out!
</div>
<div class="alert alert-light" role="alert">
<i class="material-icons">person</i> This is a light alert—check it out!
</div>
<div class="alert alert-dark" role="alert">
<i class="material-icons">person</i> This is a dark alert—check it out!
</div>You get this:
Because somebody decided it's an amazing idea to style one alert color completely differently and hack it up: .alert-warning {
.material-icons {
padding-top: $extra-small-space;
margin-right: $small-space;
font-size: 2rem;
color: $warning;
}
.alert-text {
padding-top: $small-space;
font-size: 0.9375rem;
}
etc. |
ShaiMagal
approved these changes
Dec 6, 2024
kpodemski
approved these changes
Dec 6, 2024
florine2623
approved these changes
Dec 12, 2024
There was a problem hiding this comment.
Hello @Hlavtox ,
I tested this PR with PrestaShop/PrestaShop#37018.
More details on how those 2 PR were tested here : PrestaShop/PrestaShop#37018 (review)
This is validated ✅
florine2623
added
QA ✔️
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Related core PRs
How it looks
In stock
Available on backorder
Not available (+ a new availability submessage)
Disabled (and product displayed)
