Skip to content
This repository was archived by the owner on Aug 26, 2022. It is now read-only.

Conversation

@ExE-Boss
Copy link
Contributor

@ExE-Boss ExE-Boss commented Sep 5, 2018

This is a follow‑up to mdn/kumascript#788, which added the standard .overheadIndicator class to the {{SeeCompatTable}} macro.

Depends on #4979

Copy link
Contributor

@jwhitlock jwhitlock left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @ExE-Boss.

Kuma changes require a Bugzilla ticket. I think we can reuse https://bugzilla.mozilla.org/show_bug.cgi?id=1486527, seems related to snugging.

I searched for a page where this would make a difference. https://developer.mozilla.org/en-US/docs/Web/CSS/offset-distance looked like a good candidate. However, it looks snugged already in Safari, and the change in PR #4961 will fix it for Chrome and Firefox (to be deployed today). Do you know of a page where this change will make a difference?

@ExE-Boss
Copy link
Contributor Author

This isn’t necessary for indicator snugging.

It’s necessary to be able to remove the notice class from the {{SeeCompatTable}} macro.

See mdn/kumascript#788 for details.

@jwhitlock
Copy link
Contributor

There's no mention of notice on mdn/kumascript#788, maybe you mean #4884 (comment) ? Should this PR remove some .notice rules as well then? And, do you have a page where all this matters?

@ExE-Boss
Copy link
Contributor Author

I meant specifically commit mdn/kumascript@d0c3a47.

Once this is merged, I can make a follow-up to mdn/kumascript#788, where I’d remove the notice class from the {{SeeCompatTable}} macro.

@ExE-Boss ExE-Boss changed the title Use .experimental.overheadIndicator for styling {{SeeCompatTable}} Bug 1492018: Use .experimental.blockIndicator for styling {{SeeCompatTable}} Sep 18, 2018
@ExE-Boss ExE-Boss force-pushed the styles/wiki/indicators/experimental branch from 83d5dd0 to 0445685 Compare September 19, 2018 22:11
@ExE-Boss ExE-Boss changed the title Bug 1492018: Use .experimental.blockIndicator for styling {{SeeCompatTable}} Bug 1492018: Use .blockIndicator for styling {{SeeCompatTable}} Sep 19, 2018
@ExE-Boss
Copy link
Contributor Author

review?(@jwhitlock): I’d say this is ready for review.

@schalkneethling schalkneethling self-assigned this Oct 2, 2018
@schalkneethling schalkneethling added the comp: frontend Component: Frontend label Oct 2, 2018
@jwhitlock
Copy link
Contributor

I think I'm the wrong person to review this. I can review style changes if you are trying to fix something that I can see on a particular page on MDN, because I can load the page in my browser before and after the change and see what the effect is. I'm not aware of any pages that this change would make a visual difference on.

This appears to be a refactoring of classes for some quality goal in the CSS source, which isn't clear to me. I think @schalkneethling or @hobinjk are better suited to review.

@ExE-Boss
Copy link
Contributor Author

ExE-Boss commented Oct 2, 2018

I'm not aware of any pages that this change would make a visual difference on.

This is necessary for mdn/kumascript#830 (diff).

Without this, if mdn/kumascript#830 is merged to production as is, any usage of the {{SeeCompatTable}} macro will suddenly be blue instead of yellow, as the yellow colour is currently set using the .notice class, and not the .experimental class, which is set to blue because it’s needed to be blue by {{Experimental_Inline}} (which uses <span class="inlineIndicator experimental"></span> for the styling).

@jwhitlock
Copy link
Contributor

@schalkneethling or @hobinjk, please take a look when you have a chance. There's several PRs in the same topic, and I've documented my understanding of the issue.

I'd also support closing this and related PRs, it is hard for me to justify spending more time on what appears to be low-priority cleanup work.

@schalkneethling
Copy link

As mentioned, it would be super useful if you could merge the work here into #4979

@ExE-Boss
Copy link
Contributor Author

ExE-Boss commented Oct 8, 2018

I have now locally developed a better solution than this.

@ExE-Boss ExE-Boss closed this Oct 8, 2018
schalkneethling pushed a commit that referenced this pull request Oct 17, 2018
…4979)

See [bug&nbsp;1492018](https://bugzil.la/1492018) for details.

## TODO
- [x] Add the&nbsp;`.properties` class to&nbsp;the&nbsp;`:matches(…)` selector (see [`ime-mode`](https://developer.mozilla.org/docs/Web/CSS/ime-mode) for a&nbsp;case where this is&nbsp;necessary)
- [x] Push remaining commits (3/3)

## Changes:
- Add support for snugging `.blockIndicator`.
- Add indicator colour override classes (obsoletes&nbsp;#4962):
  - `.indicator-info` - grey
  - `.indicator-version` - blue
  - `.indicator-warning` - yellow
  - `.indicator-danger` - red
- Use classes instead of `style` attributes for ES7/Harmony and JSOverrides.
- Fix `<pre>` block and `.properties` table snugging.

---

review?(@schalkneethling)
@ExE-Boss ExE-Boss deleted the styles/wiki/indicators/experimental branch October 19, 2018 13:24
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

comp: frontend Component: Frontend

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants