Skip to content
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

[css-grid] Report a warning if duplicate area names are detected #1038

Closed
Dan503 opened this issue May 10, 2018 · 15 comments
Closed

[css-grid] Report a warning if duplicate area names are detected #1038

Dan503 opened this issue May 10, 2018 · 15 comments

Comments

@Dan503
Copy link
Contributor

Dan503 commented May 10, 2018

Take this css:

.grid-alpha {
  grid-template-areas: "delta  echo";
}

.grid-beta {
  grid-template-areas: "echo  delta";
}

.grid-cell {
  -ms-grid-column: ???; /* what column does .grid-cell go in? */
  arid-area: echo;
}

Autoprefixer has no access to the DOM so it doesn't really know what area .grid-cell belongs to.

Currently Autoprefixer silently just goes with whatever the last grid-template-areas definition was in the style-sheet (in this case "echo delta"). If a user places .grid-cell in .grid-alpha then it will look great in modern browsers but be placed in the wrong column in IE.

Since it is silent, a user could end up building their whole site blissfully unaware of this issue, using duplicate area names everywhere, then finally open IE to see their whole layout go to shit.

Autoprefixer should detect if there are any area name conflicts and if so, warn users to use unique area names at all times.

@Dan503 Dan503 changed the title Report a warning if duplicate area names are detected [css-grid] Report a warning if duplicate area names are detected May 10, 2018
@yepninja
Copy link
Collaborator

Good suggestion! I think we can do it.

@Dan503
Copy link
Contributor Author

Dan503 commented May 16, 2018

Oh I just realised that you will need to be careful not to trigger false positives on media queries.

This code would be valid:

.grid {
  display: grid;
  grid-gap: 10px;
  grid-template:
    "a   b" 100px
    "c   d" 100px
    "e   f" 100px /
    1fr  1fr;
}

@media (min-width: 600px){
  .grid {
    grid-template-areas:
      "a   b   c"
      "d   e   f";
    grid-template-columns: repeat(3, 1fr);
    grid-template-rows: repeat(2, 100px);
  }
}

.cell-A {
  grid-area: a;
}

.cell-B {
  grid-area: b;
}

.cell-C {
  grid-area: c;
}

.cell-D {
  grid-area: d;
}

@yepninja
Copy link
Collaborator

@Dan503 grid-template-areas in media is processed separately, so it's not a problem.

@Dan503
Copy link
Contributor Author

Dan503 commented May 16, 2018

Not if issue #1045 is fixed.

I think it would be safe though to just ignore area names that are inside media queries and only report a warning if duplicates are found outside of media queries.

update: I've posted a better idea further down.

@ShaggyDude
Copy link

+1

@ai
Copy link
Member

ai commented Jul 6, 2018

I sent this task to our Cult of Martians.

@Dan503
Copy link
Contributor Author

Dan503 commented Jul 6, 2018

I'm thinking that if a duplicate area name is found in a media query, if the selector for the element is different then it should trigger the warning.

It is likely to cause some false positives but it will also likely catch out a lot more true positives than ignoring media queries completly.

@Dan503
Copy link
Contributor Author

Dan503 commented Jul 6, 2018

For comma seperated lists, each comma should be assessed separately as to weather the selector matches or not.

@ai
Copy link
Member

ai commented Jul 22, 2018

Fixed #1080

@jdalegonzalez
Copy link

Forgive me if I'm missing something but isn't this:

grid-template-areas:
    "Header           Header   Header"
    "TableOfContents  Tabs     Sidebar"
    "TableOfContents  Main     Sidebar"
    "Footer           Footer   Footer";

the way you're supposed to describe a grid with what amounts to rowSpan and colSpan sections? Something like....

+===============================+
|           Header              |
+=======+==============+========+
|       |     Tabs     |        |
|       +==============+        |
|       |              |        |
| TOC   |              |  Side  |
|       |     Main     |        |
|       |              |        |
|       |              |        |
+=======+==============+========+
|           Footer              |
+===============================+

Ref: https://developer.mozilla.org/en-US/docs/Web/CSS/grid-template-areas

Are you saying that while it's legal, it isn't supported by IE? It seems like a problem that legal CSS generates a warning. Is there a better way to accomplish the layout and still get autoprefixer support?

@ai
Copy link
Member

ai commented Aug 3, 2018

@jdalegonzalez it is OK.

We spoke about same are names in two different grid-template.

@Dan503
Copy link
Contributor Author

Dan503 commented Aug 3, 2018

@jdalegonzalez read the issue carefully. The problem isn't duplicate area names being used in the same template declaration.

The problem comes in when two completely different grid elements try to use the same area names. Autoprefixer gets confused over what grid cells need to go in what grids.

@Dan503
Copy link
Contributor Author

Dan503 commented Aug 4, 2018

@ai I've removed the references to this issue in the CSS Tricks article since it has been fixed now.

@jdalegonzalez
Copy link

jdalegonzalez commented Aug 6, 2018

@Dan503 Thanks for the clarification. I think what was tripping me up is that error reads "duplicate area names detected in rule: rule with issue", making it feel like the problem was scoped to the single rule as opposed to something like, "duplicate area names detected in rules: first rule defining area and second rule defining area", which emphasizes the fact that it's a conflict between two rules. Adding to my confusion was that in this particular case, the two rules would never conflict because we either assign one css class or the other, never both. But I get that there is no way for autoprefixer to know that.

@Dan503
Copy link
Contributor Author

Dan503 commented Aug 6, 2018

@jdalegonzalez actually we've figured out how to safely support duplicate area names now!

Take a look at this github issue, we are working on implementing it right now :)
#1091

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

No branches or pull requests

5 participants