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

Bug 1492018: Prefer `.blockIndicator` for styling block indicators #830

Merged
merged 3 commits into from Oct 17, 2018

Conversation

Projects
None yet
4 participants
@ExE-Boss
Contributor

ExE-Boss commented Sep 19, 2018

@ExE-Boss ExE-Boss force-pushed the ExE-Boss:macros/bug1492018-block-indicator branch from 052431d to f99e26a Sep 20, 2018

@ExE-Boss ExE-Boss force-pushed the ExE-Boss:macros/bug1492018-block-indicator branch from 4896b05 to d70c762 Sep 20, 2018

@ExE-Boss ExE-Boss force-pushed the ExE-Boss:macros/bug1492018-block-indicator branch from d70c762 to 2b3aee7 Sep 28, 2018

@wbamberg

This comment has been minimized.

Member

wbamberg commented Oct 2, 2018

As noted in https://bugzilla.mozilla.org/show_bug.cgi?id=1492018, @schalkneethling is probably the best person to review this.

@@ -12,7 +12,6 @@ var str = mdn.localString({
"ru" : "<strong>Это экспериментальная технология</strong><br />Так как спецификация этой технологии ещё не стабилизировалась, смотрите <a href='#Browser_compatibility'>таблицу совместимости</a> по поводу использования в различных браузерах. Также заметьте, что синтаксис и поведение экспериментальной технологии может измениться в будущих версиях браузеров, вслед за изменениями спецификации."
});
%><div class="notice overheadIndicator experimental">
%><div class="blockIndicator experimental">

This comment has been minimized.

@ExE-Boss

ExE-Boss Oct 2, 2018

Contributor

This change depends on mozilla/kuma#4962.

This comment has been minimized.

@schalkneethling

schalkneethling Oct 8, 2018

Contributor

Can you please merge this change in mozilla/kuma#4979

It makes it difficult to review this change in it's entirety if we have tow separate Kuma pull requests that this Kumascript change depends on.

@schalkneethling

A few suggestions, comments and requested changes. Thanks @ExE-Boss

@@ -18,6 +18,6 @@ var s_msg = mdn.localString({
var str = s_msg[0] + link + s_msg[1];
%>
<div class="overheadIndicator htmlVer htmlMinVerHeader">
<div class="blockIndicator htmlVer htmlMinVerHeader">

This comment has been minimized.

@schalkneethling

schalkneethling Oct 8, 2018

Contributor

Doing a search using the Active Macros utility, there are no document still using the macro:
https://developer.mozilla.org/en-US/search?topic=none&kumascript_macros=HTMLVersionHeader&locale=*

Perhaps not worth updating it then?

This comment has been minimized.

@ExE-Boss

ExE-Boss Oct 8, 2018

Contributor

This just got removed in #927.

This comment has been minimized.

@schalkneethling

schalkneethling Oct 8, 2018

Contributor

Ok, so a rebase will then remove it from this PR as well. Thanks!

@@ -12,7 +12,6 @@ var str = mdn.localString({
"ru" : "<strong>Это экспериментальная технология</strong><br />Так как спецификация этой технологии ещё не стабилизировалась, смотрите <a href='#Browser_compatibility'>таблицу совместимости</a> по поводу использования в различных браузерах. Также заметьте, что синтаксис и поведение экспериментальной технологии может измениться в будущих версиях браузеров, вслед за изменениями спецификации."
});
%><div class="notice overheadIndicator experimental">
%><div class="blockIndicator experimental">

This comment has been minimized.

@schalkneethling

schalkneethling Oct 8, 2018

Contributor

Can you please merge this change in mozilla/kuma#4979

It makes it difficult to review this change in it's entirety if we have tow separate Kuma pull requests that this Kumascript change depends on.

Show resolved Hide resolved macros/SimpleBadge.ejs
Show resolved Hide resolved macros/SimpleBanner.ejs
@@ -18,6 +18,6 @@ var s = mdn.localString({
});
%>
<div class="overheadIndicator" style="background:#9CF49C;">
<div class="blockIndicator" style="background:#9CF49C;">

This comment has been minimized.

@schalkneethling

schalkneethling Oct 8, 2018

Contributor

This one breaks because there is a very specific selector that combines overheadIndicator with the exact style attribute and value.

screenshot 2018-10-08 at 12 14 34

You can see an example of that here:
https://github.com/mozilla/kuma/blob/257a32b677d3db54fef2de38e91958bb7bbdae6a/kuma/static/styles/components/wiki/indicators.scss#L212

As you are working on improving this, it seems like it would make sense to refactor this so it no longer has this dependency. Instead it should follow the pattern of two classes indicating block/inline and icon, color etc.

This comment has been minimized.

@schalkneethling

schalkneethling Oct 12, 2018

Contributor

This one is still displaying incorrectly. I noticed that you removed the selector that was looking for the style attribute with the specific colour value, but this macro should be updated as follows:

<div class="blockIndicator es-harmony">
@@ -1,4 +1,4 @@
<div class="inheritsbox template-jsOverrides" style="border: 5px solid #D1D1FF; background: #f5f5ff; padding: 2px 10px; margin: 25px 0; overflow: hidden;">
<div class="blockIndicator inheritsbox template-jsOverrides" style="border: 5px solid #D1D1FF; background: #f5f5ff; padding: 2px 10px; margin: 25px 0; overflow: hidden;">

This comment has been minimized.

@schalkneethling

schalkneethling Oct 8, 2018

Contributor

Here as well I would suggest removing the style attribute and moving the styling to a class inside Kuma.

Show resolved Hide resolved macros/minversionGeneric.ejs
Show resolved Hide resolved macros/non-standardGeneric.ejs
Show resolved Hide resolved macros/unimplementedGeneric.ejs
Show resolved Hide resolved tests/macros/test-draft.js
@schalkneethling

This comment has been minimized.

Contributor

schalkneethling commented Oct 8, 2018

@ExE-Boss seeing that there is a number of macros here that are used by other macros. Can you suggest some pages I can use to test those other than the list of pages I specified in #830 (comment)

@ExE-Boss

This comment has been minimized.

Contributor

ExE-Boss commented Oct 8, 2018

(In reply to @schalkneethling from #830 (comment))

@ExE-Boss seeing that there is a number of macros here that are used by other macros. Can you suggest some pages I can use to test those other than the list of pages I specified in #830 (comment)

I just created https://developer.mozilla.org/docs/Sandbox/Indicators specifically for this reason.

@ExE-Boss ExE-Boss force-pushed the ExE-Boss:macros/bug1492018-block-indicator branch from 2b3aee7 to 5972fe9 Oct 8, 2018

@ExE-Boss

This comment has been minimized.

Contributor

ExE-Boss commented Oct 8, 2018

@schalkneethling: This is ready for a second round of review.

@ExE-Boss ExE-Boss force-pushed the ExE-Boss:macros/bug1492018-block-indicator branch from 5972fe9 to 3b452a3 Oct 8, 2018

@@ -12,7 +12,6 @@ var str = mdn.localString({
"ru" : "<strong>Это экспериментальная технология</strong><br />Так как спецификация этой технологии ещё не стабилизировалась, смотрите <a href='#Browser_compatibility'>таблицу совместимости</a> по поводу использования в различных браузерах. Также заметьте, что синтаксис и поведение экспериментальной технологии может измениться в будущих версиях браузеров, вслед за изменениями спецификации."
});
%><div class="notice overheadIndicator experimental">
%><div class="blockIndicator experimental indicator-warning">

This comment has been minimized.

@ExE-Boss

ExE-Boss Oct 8, 2018

Contributor

mozilla/kuma#4979 now adds the following indicator color override classes:

  • .indicator-info - grey
  • .indicator-version - blue
  • .indicator-warning - yellow
  • .indicator-danger - red
@schalkneethling

This comment has been minimized.

Contributor

schalkneethling commented Oct 9, 2018

Thanks for the update @ExE-Boss, will review as soon as possible.

@schalkneethling

Getting very close. One item that needs to be changed. Thanks @ExE-Boss

@ExE-Boss ExE-Boss referenced this pull request Oct 16, 2018

Merged

rm es7 #948

@schalkneethling

This comment has been minimized.

Contributor

schalkneethling commented Oct 17, 2018

This still needs approval from someone with write access. @Elchi3 can you do that based on @jwhitlock comment here mozilla/kuma#4979 (comment)

@Elchi3

Elchi3 approved these changes Oct 17, 2018

I haven't actually been involved here or tested this, but rubber-stamping so you are able to go ahead.

@ExE-Boss

This comment has been minimized.

Contributor

ExE-Boss commented Oct 17, 2018

This still needs merging from someone with write access.

@Elchi3 Elchi3 merged commit 938e4de into mdn:master Oct 17, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@ExE-Boss ExE-Boss deleted the ExE-Boss:macros/bug1492018-block-indicator branch Oct 17, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment