Bug 1492018: Prefer .blockIndicator for styling block indicators#4979
Conversation
|
@ExE-Boss please rebase this against the latest in master. There are some display problems caused by this branch being out of sync with some recent changes. Thanks! |
ExE-Boss
force-pushed
the
styles/wiki/indicators/bug1492018-block-indicator
branch
from
53ee3b0 to
9876ccb
Compare
ExE-Boss
force-pushed
the
styles/wiki/indicators/bug1492018-block-indicator
branch
from
9876ccb to
637908e
Compare
|
@schalkneethling: This is ready for a second round of review. |
ExE-Boss
force-pushed
the
styles/wiki/indicators/bug1492018-block-indicator
branch
2 times, most recently
from
6f55ee5 to
495a940
Compare
ExE-Boss
force-pushed
the
styles/wiki/indicators/bug1492018-block-indicator
branch
from
495a940 to
0e3a01f
Compare
|
Thanks for the update @ExE-Boss, will review as soon as possible. |
schalkneethling
left a comment
schalkneethling
left a comment
There was a problem hiding this comment.
Couple of items remaining. Getting very close. Thanks @ExE-Boss
| @include indicator-icon('\f017'); /* clock */ | ||
| } | ||
|
|
||
| .esHarmony, |
There was a problem hiding this comment.
We need to standardize on how classes are written. For the vast majority of the code base, and pretty much the standard across projects, class names should follow the pattern descriptive-word and not the JS variable form of descriptiveWord
Therefore this needs to be updated to be es-harmony. There are a couple of classes here that does not follow that convention, we can clean this up in a follow on pull request.
|
|
||
| /* es7 and harmony macros */ | ||
| .esHarmony { | ||
| @include box-theme('positive'); |
There was a problem hiding this comment.
I feel like we need to be consistent with the colors we assign to these callout widgets. Experimental feel to me more like a blue, or even orange, than a green.
Here for example it is blue:
http://localhost:8000/en-US/docs/Web/API/DeviceLightEvent/Using_light_sensors
But then on the Object.observe page locally it is green:
http://localhost:8000/tr/docs/Web/JavaScript/Reference/Global_Objects/Object/observe
I would suggest that we keep this consistent, and rather opt for blue in experimental cases.
There was a problem hiding this comment.
I think I’ll just use the .experimental and .indicator-warning classes for {{ES7}} to match {{SeeCompatTable}}, also {{ES7}} is used very little, so it may very well end up on the cutting room floor as part of the KumaScript Macro Massacre project.
|
@schalkneethling: This is ready for a third round of review. |
|
So, can this be merged now? |
|
Ok, I have tested this on a couple of pages(will do some more testing) where I have the new styles in Kuma, but the old macros in kumascript. I cannot find any instances where things fall apart. So, I believe we are safe to merge this and the KumaScript PR and push to production. For the space of time where we will have the old macros and new classes(until the pages have been regenerated) all should be fine. With that said, we should probably hold of until Monday when @jwhitlock is back. |
I don't think this needs to wait for me. It appears to me the worst that can happen is a banner is un-styled, and @ExE-Boss can then either force-refresh a page or edit the content to add the class. |
|
So then let’s merge this and mdn/kumascript#830. (also I’ve had this loaded in Stylus for the last week or so and everything looks fine) |
ExE-Boss
deleted the
styles/wiki/indicators/bug1492018-block-indicator
branch
…xes (#5079) I didn’t know how to do this in #4979 until @schalkneethling opened #5069. review?(@chrisdavidmills, @jwhitlock)
3 participants
See bug 1492018 for details.
TODO
.propertiesclass to the:matches(…)selector (seeime-modefor a case where this is necessary)Changes:
.blockIndicator..blockIndicatorfor styling{{SeeCompatTable}}#4962):.indicator-info- grey.indicator-version- blue.indicator-warning- yellow.indicator-danger- redstyleattributes for ES7/Harmony and JSOverrides.<pre>block and.propertiestable snugging.review?(@schalkneethling)