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

Bug 1486527: Use :is(…) selector and PostCSS to improve indicator snugging #4884

Conversation

ExE-Boss
Copy link
Contributor

@ExE-Boss ExE-Boss commented Jul 15, 2018

Fixes bug 1486527

Reasoning behind this change

When I added the {{deprecated_header}} macro to Object.prototype.__proto__, I had to wrap all the indicator headers in a <div> to prevent the empty <p> from being inserted between them, and I also had to add the overheadIndicator class to the first warning so that it would get snugged up to the deprecation header.

I also had similar problems with some indicators not getting snugged up in the past, and I found it annoying.

TODO:

  • Either find or open a bug for this PR.

@jwhitlock
Copy link
Contributor

Thanks @ExE-Boss. gulp is used for front-end development for speed, but a different toolchain is used to create the production assets. When I use make build-static in the Docker container, the change in wiki.css is:

/* heading */
/* If there are 2 indicators in close proximity, snug them up.
   Too many combinations to do them all but this
   should take care of most common combinations */
:matches(.warning):matches(.overheadIndicator):matches(.note) + :matches(.warning):matches(.overheadIndicator):matches(.note),
:matches(.warning):matches(.overheadIndicator):matches(.note) + p:empty + :matches(.warning):matches(.overheadIndicator):matches(.note) {
  margin-top: -15px; }

Is this what you are expecting? Or are there supposed to be additional rules for Edge and Firefox?

@ExE-Boss
Copy link
Contributor Author

Is this what you are expecting? Or are there supposed to be additional rules for Edge and Firefox?

It’s supposed to be expanded using postcss and postcss‑selector‑matches into:

/* If there are 2 indicators in close proximity, snug them up.
   Too many combinations to do them all but this
   should take care of most common combinations */
.warning + .warning,
.overheadIndicator + .warning,
.note + .warning,
.warning + .overheadIndicator,
.overheadIndicator + .overheadIndicator,
.note + .overheadIndicator,
.warning + .note,
.overheadIndicator + .note,
.note + .note,
.warning + p:empty + .warning,
.overheadIndicator + p:empty + .warning,
.note + p:empty + .warning,
.warning + p:empty + .overheadIndicator,
.overheadIndicator + p:empty + .overheadIndicator,
.note + p:empty + .overheadIndicator,
.warning + p:empty + .note,
.overheadIndicator + p:empty + .note,
.note + p:empty + .note {
  margin-top: -15px; }

@jwhitlock
Copy link
Contributor

Sorry, we have quite a few things to fix before we can add PostCSS to our production asset toolchain. That's tracked in bug 1366868 (PostCSS) and bug 1380419 (Refactor static asset generation), but both bugs are a bit stale. We have a number of front-end changes planned, so I'm optimistic we'll do this work this quarter.

@ExE-Boss ExE-Boss changed the title Use :matches(…) selector and PostCSS to improve indicator snugging Bug 1486527 - Use :matches(…) selector and PostCSS to improve indicator snugging Aug 27, 2018
@jwhitlock
Copy link
Contributor

Thanks for bug 1486527, and updating this PR.

The same package.json is now used in Docker and on the local host (PR #4852). There were a few updates, and so now there is a merge conflict that needs to be resolved. However, we still would need to migrate from django-pipeline to gulp in the Docker image to use PostCSS. We're at least a month away from that, if we prioritize it. I've got a few people to convince that it is a priority.

@ExE-Boss - how do you feel about getting the "compiled" version into indicators.scss, with a note that it would be shorter with PostCSS? That way, we'd get the better indicator snugging today, and cleaner source files in the future.

@ExE-Boss
Copy link
Contributor Author

(in reply to @jwhitlock from #4884 (comment))

how do you feel about getting the "compiled" version into indicators.scss, with a note that it would be shorter with PostCSS? That way, we'd get the better indicator snugging today, and cleaner source files in the future.

All right, I’ll add the compiled version before the :matches(…) version (I also found out that I have to add .notice.experimental to the list, unless mdn/kumascript#788 is merged first).

@ExE-Boss
Copy link
Contributor Author

ExE-Boss commented Aug 29, 2018

review?(@jwhitlock): I’ve added pre and .notice to the :matches(…) selector, as .notice is needed by the {{SeeCompatTable}} macro (see mdn/kumascript#788) and pre because I have encountered places where an .overheadIdicator was directly followed by a pre tag.

@jwhitlock
Copy link
Contributor

I've cherry-picked the CSS changes, including the expanded CSS, into PR #4958.

I'm uncomfortable bringing in PostCSS and plugins, because I think it is confusing that you can use them for local front-end development but that they aren't applied for the CSS that shows up on MDN. Also, package-lock.json would need to be updated, see https://kuma.readthedocs.io/en/latest/development.html#front-end-toolchain-dependencies.

I still plan on fixing bug 1366868 (PostCSS) by the end of the year, and I think this PR is a good demonstration of why we'd want it.

@jwhitlock
Copy link
Contributor

With the time remaining in the year, PostCSS won't be until next year, Q1 or Q2.

@ExE-Boss ExE-Boss changed the title Bug 1486527 - Use :matches(…) selector and PostCSS to improve indicator snugging Bug 1486527: Use :is(…) selector and PostCSS to improve indicator snugging Dec 28, 2018
@jwhitlock
Copy link
Contributor

Let's revisit this idea when we have a new asset pipeline, and when the standards folks decide what the selector should look like.

@jwhitlock jwhitlock closed this Jan 7, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants