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

Show warning if grid-area has a non-unique identifier (fixes #1038) #1080

Merged
merged 8 commits into from Jul 30, 2018

Conversation

@bogdan0083
Copy link
Contributor

@bogdan0083 bogdan0083 commented Jul 22, 2018

This is my first PR which adds warning when grid-area has a non-unique identifier (conflict of names). #1038

Not sure if code is clean enough, but I had a lot of fun working on this issue. Thanks! :)

Some questions:

  • In my code I refer to area names in grid-area as "identifiers" (see this). Is it alright?
  • How should we handle grid-area inside media? I've omitted checking warnings inside media for now
@ai
Copy link
Member

@ai ai commented Jul 22, 2018

The code is great 👍

@Dan503 @yepninja what do you think?

@ai
Copy link
Member

@ai ai commented Jul 22, 2018

@bogdan0083 why it doesn’t see name conflict between .c and .hd?

@bogdan0083
Copy link
Contributor Author

@bogdan0083 bogdan0083 commented Jul 23, 2018

@ai I double-checked the test case and I'm sure there's no conflict here. hd identifier is unique (there's no other grid-template properties with "hd")

@Dan503
Copy link
Contributor

@Dan503 Dan503 commented Jul 23, 2018

I'll take a close look at this tonight to see if it works how I expect it to.

@ai
Copy link
Member

@ai ai commented Jul 23, 2018

Yeap, it is my mistake :(. There are no problems right now. Great work.

Let’s wait @Dan503 review (anyway I want to wait for #1081 before releasing it).

@Dan503
Copy link
Contributor

@Dan503 Dan503 commented Jul 23, 2018

... ummm... ok I'm stumped.

How exactly do I go about testing this pull request? 😕

@bogdan0083
Copy link
Contributor Author

@bogdan0083 bogdan0083 commented Jul 23, 2018

Whoops. I've just pushed a commit. Did I break something?

@Dan503
Copy link
Contributor

@Dan503 Dan503 commented Jul 23, 2018

No, (or well I don't know) I just don't know how to test :P

@bogdan0083
Copy link
Contributor Author

@bogdan0083 bogdan0083 commented Jul 23, 2018

Alright. In the last commit I've added warning for a grid-area even inside media rules. It should trigger inside a media in the following cases:

  • If we're can't find the same selector outside media
  • If we found the same selector, but outside grid-area value is different

In all other cases a warning will not be shown.

@Dan503
Copy link
Contributor

@Dan503 Dan503 commented Jul 23, 2018

I've taken a look at your test cases file. It is missing media query tests. It is also missing comma separated list tests.

/* Your current tests */
.g {
    display: grid;
    grid-gap: 10px;
    grid-template:
        "g   g" 100px
        "g   g" 100px
        "h   h" 100px /
        1fr  1fr;
}

.g-conflict {
    display: grid;
    grid-gap: 10px;
    grid-template:
        "g   g" 100px
        "g   g" 100px
        "h   h" 100px /
        1fr  1fr;
}

@media (min-width: 600px) {
  /* This should *not* trigger a warning */
  .g {
    display: grid;
    grid-gap: 10px;
    grid-template:
        "g   h" 100px /
        1fr  1fr;
  }

  /* This *should* trigger a warning */
  .g-conflict-2 {
    display: grid;
    grid-gap: 10px;
    grid-template:
        "g   h" 100px /
        1fr  1fr;
  }
}

/* comma list tests */
/* None of these should throw any warnings (unless specified) */
.i, .j {
  display: grid;
  grid-gap: 10px;
  grid-template:
      "i    j" 100px /
      1fr  1fr;
}

@media (max-width: 600px) {
  .i {
    grid-template:
      "i" 100px
      "j" 100px /
      1fr;
  }
  .j {
    grid-template:
      "i" 100px
      "j" 100px /
      1fr;
  }

  /* This one should throw a warning */
  .k {
    grid-template:
      "i" 100px
      "j" 100px /
      1fr;
  }
}

@media (min-width: 900px) {
  .i, .j {
    display: grid;
    grid-gap: 10px;
    grid-template:
      "i" 100px
      "j" 100px /
      1fr;
  }
}

/* media query test */
@media (min-width: 601px) {
  .l {
    display: grid;
    grid-gap: 10px;
    grid-template:
        "l   m" 100px /
        1fr  1fr;
  }
}
@media (max-width: 600px) {
  .l {
    display: grid;
    grid-gap: 10px;
    grid-template:
        "l   m" 100px /
        1fr  1fr;
  }

  /* this should display a warning */
  .z {
    display: grid;
    grid-gap: 10px;
    grid-template:
        "m   z" 100px /
        1fr  1fr;
  }
}

That's all the tests I can think of.

@bogdan0083
Copy link
Contributor Author

@bogdan0083 bogdan0083 commented Jul 23, 2018

Alright, thanks! Looks like I chose a completely wrong approach. I triggered warning only when grid-area is used (which is not very good as I understood now).

Now I'll just take your code above as test case and show warnings every time there are duplicate area names 👍

@Dan503
Copy link
Contributor

@Dan503 Dan503 commented Jul 23, 2018

I triggered warning only when grid-area is used (which is not very good as I understood now).

Having display: grid; in the same rule should not be a requirement for the warning to not go off. It should still be powered pretty much entirely through the grid-template-areas property if possible.

@Dan503
Copy link
Contributor

@Dan503 Dan503 commented Jul 23, 2018

Yeah the media query and comma seperated list support is what makes this issue much more difficult to fix than it appears to be on the surface.

@bogdan0083
Copy link
Contributor Author

@bogdan0083 bogdan0083 commented Jul 24, 2018

Alright, looks like it should work as expected inside media rules now 😄

@Dan503 Can you please check the last commit in your spare time please? These are the warnings I get from the last grid-template.css:

"autoprefixer: <css input>:41:5: duplicate area names: head, nav, main, foot",
"autoprefixer: <css input>:133:5: duplicate area names: g, h",
"autoprefixer: <css input>:154:5: duplicate area names: g, h",
"autoprefixer: <css input>:186:5: duplicate area names: i, j",
"autoprefixer: <css input>:228:5: duplicate area name: m",

And then we can go for a code review! :)

@Dan503
Copy link
Contributor

@Dan503 Dan503 commented Jul 25, 2018

If those are the warnings that those tests are giving off now, then it sounds like it's working how it's supposed to :)

I feel like the warning message isn't detailed enough though. It warns that duplicate area names are used but it doesn't tell users why duplicate area names are bad or what CSS selector is causing the issue.

If the warning could say something more like this then I think it would be better:

autoprefixer: <css input>:133:5:
duplicate area names detected in rule: .g-conflict
duplicate area names: g, h
duplicate area names cause unexpected behavior in IE
@Dan503
Copy link
Contributor

@Dan503 Dan503 commented Jul 25, 2018

You can use \n to create the line breaks, or template literals if ES6 is an option.

@bogdan0083
Copy link
Contributor Author

@bogdan0083 bogdan0083 commented Jul 25, 2018

Alright, done 👍

@ai Now we can review the code I guess (or we can wait until #1081 is fixed, doesn't matter). I changed the implementation completely compared to the initial commit.

@ai
Copy link
Member

@ai ai commented Jul 25, 2018

Awesome 👍

I will review it on this weekend.

@Dan503
Copy link
Contributor

@Dan503 Dan503 commented Jul 26, 2018

I just took a look at the commit. You are using one big template literal which is ok but you need to factor in prefered output white space rather than whatever whitespace lines up nicely with the rest of the code in the file.

The warning will currently be outputting this:

autoprefixer: <css input>:133:5: 
        duplicate area names detected in rule: .g-conflict
        duplicate area names: g, h
        duplicate area names cause unexpected behavior in IE

A bit of an indent is ok but 8 spaces worth is a bit much :/

A large indent will cause it to go off the screen or wrap in peoples console windows earlier than it needs to.

I'd recommend reducing the indent down to 2 spaces.

autoprefixer: <css input>:133:5: 
  duplicate area names detected in rule: .g-conflict
  duplicate area names: g, h
  duplicate area names cause unexpected behavior in IE
@bogdan0083
Copy link
Contributor Author

@bogdan0083 bogdan0083 commented Jul 26, 2018

I just took a look at the commit. You are using one big template literal which is ok but you need to factor in prefered output white space rather than whatever whitespace lines up nicely with the rest of the code in the file.

I've been aware of this issue and I thought maybe someone would give me an advice on fixing the indentation (but forgot to ask).

In the code I use a postcss warn method to show a warning:

      decl.warn(
        result,
        `
        duplicate area ${ word } detected in rule: ${ rule.selector }
        duplicate area ${ word }: ${ duplicates.join(', ') }
        duplicate area names cause unexpected behavior in IE`
      )

the part autoprefixer: <css input>:133:5: adds automatically before the string.

If I want to indent warning normally, I have to format a warning method like this:

       decl.warn(
        result,
        `
     duplicate area ${ word } detected in rule: ${ rule.selector }
     duplicate area ${ word }: ${ duplicates.join(', ') }
     duplicate area names cause unexpected behavior in IE`
      ) 

which is quite an ugly way to indent code.
And the same for the testing code.

If I use normal string like 'string' + '\n' it still breaks the indentation.

I searched for every warning inside autorefixer – they all are one-liners. I even checked the testing code for warn method (in postcss repository) – there are no multi-line warnings in there as well.

I guess we need to wait what Andrey says on how to fix this (or someone who knows how to make warnings multi-line).

@Dan503
Copy link
Contributor

@Dan503 Dan503 commented Jul 26, 2018

If you don't want to deal with the ugliness, try this instead. It should produce the output with a 2 space indent.

      decl.warn(
        result,
        ["",
         `  duplicate area ${ word } detected in rule: ${ rule.selector }`,
         `  duplicate area ${ word }: ${ duplicates.join(', ') }`,
         `  duplicate area names cause unexpected behavior in IE`].join('\n')
      )

It's up to @ai if he's ok with a multi-line warning in autoprefixer though.

@bogdan0083
Copy link
Contributor Author

@bogdan0083 bogdan0083 commented Jul 26, 2018

If you don't want to deal with the ugliness, try this instead. It should produce the output with a 2 space indent.

Thank you very much! Such a simple solution 😄

It's up to @ai if he's ok with a multi-line warning in autoprefixer though.

I agree that multi-line warning is more informative and nicer. If warnings must be one-liners it's not much of a problem to return warnings back to a single line

@ai
Copy link
Member

@ai ai commented Jul 29, 2018

Are we ready for merge and everything was fixed in the last commit?

@bogdan0083
Copy link
Contributor Author

@bogdan0083 bogdan0083 commented Jul 30, 2018

@ai yep, I finished working on this PR. If the code is fine we can merge.

@Dan503
Copy link
Contributor

@Dan503 Dan503 commented Jul 30, 2018

Yep, I'm happy for it to be merged :)

@ai ai merged commit 8bea281 into postcss:master Jul 30, 2018
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@ai
Copy link
Member

@ai ai commented Jul 30, 2018

Released in 9.0.2

@ShaggyDude
Copy link

@ShaggyDude ShaggyDude commented Aug 1, 2018

This is amazing work. Thank you all!

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

Successfully merging this pull request may close these issues.

None yet

4 participants
You can’t perform that action at this time.