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

Add waring to align/justify-items in IE #1123

Closed
wants to merge 7 commits into from

Conversation

@janczer
Copy link
Contributor

@janczer janczer commented Sep 20, 2018

Fix #1123

@ai
Copy link
Member

@ai ai commented Sep 20, 2018

Did you show this warning only if IE is selected in Browserslist?

@janczer
Copy link
Contributor Author

@janczer janczer commented Sep 20, 2018

I think I don't. Can you please tell me where I can check if IE is selected?

@ai
Copy link
Member

@ai ai commented Sep 20, 2018

I am right now on conference. Check other Grid warnings.

@Dan503

This comment has been minimized.

Copy link
Contributor

@Dan503 Dan503 commented on test/autoprefixer.test.js in 6241b4c Sep 20, 2018

The warning message needs to be different depending on if the user used align/justify-content or align/justify-items.

align/justify-content is outright not supported so saying IE does not support it is fine.

align/justify-items is often easily able to be replicated using align/justify-self on the child items. The warning should say that align-items and justify-items are not supported in IE but advise the user to try using align-self or justify-self on the child elements instead.

This comment has been minimized.

Copy link
Contributor

@Dan503 Dan503 replied Sep 20, 2018

.grid-selector > * is often the easiest way to achieve this on the align-self and justify-self properties.

The warning should mention that I think.

@Dan503

This comment has been minimized.

Copy link
Contributor

@Dan503 Dan503 commented on test/cases/grid.css in 6241b4c Sep 20, 2018

The only test case here is align-items.

There needs to also be test cases for the other 3 properties.

  • justify-items
  • justify-content
  • align-content
…dd tips to waring to align/justify-items/content.
@janczer
Copy link
Contributor Author

@janczer janczer commented Sep 21, 2018

@Dan503 I added a tips and a test to almost all properties. Thanks for your descriptions.
But unfortunatly I found the bug #1127 and I cannot add to grid.css propertie align-content because two tests are conflicts.

@ai I was trying to find how I can add warning if specific browser is add, but I'm failing. When you will have some time, can you please describe me how I can do it? Thanks

Now I see that tests on Node 10.11.0 are failing because size of the package is too big. Should I cut it off somehow?

@ai
Copy link
Member

@ai ai commented Sep 21, 2018

How to check IE is here:
https://github.com/postcss/autoprefixer/blob/master/lib/processor.js#L101

Just increase size limit

@ai
Copy link
Member

@ai ai commented Sep 21, 2018

let grid = this.prefixes.add['grid-area']
if (this.prefixes.options.grid && grid) {
@Dan503
Copy link
Contributor

@Dan503 Dan503 commented Sep 21, 2018

@janczer I'm making some alterations to it and I'll submit it as a pull request against your branch.

@Dan503
Copy link
Contributor

@Dan503 Dan503 commented Sep 21, 2018

let grid = this.prefixes.add['grid-area']
if (this.prefixes.options.grid && grid) {

@ai was that for testing if the declaration is in a grid element?

@ai
Copy link
Member

@ai ai commented Sep 21, 2018

@Dan503 it tests that we have a browsers which needs Grid prefixes

Dan503 and others added 4 commits Sep 21, 2018
- Triggers on more than just "display: grid"
- Personalizes the "align/justify-self" suggestion based on what the user entered
- Splits the warning message into multiple lines to make the warning easier to read
@janczer
Copy link
Contributor Author

@janczer janczer commented Sep 21, 2018

@Dan503 Looks like your commits affected the old tests. Can you check it?

@Dan503
Copy link
Contributor

@Dan503 Dan503 commented Sep 21, 2018

Yeah autoprefixer.test.js needs updating to match the new output. I'm not sure how to generate the result to match it to.

I mentioned that in the pull request.

@ai
Copy link
Member

@ai ai commented Sep 21, 2018

@Dan503 only by hands 😅

@janczer
Copy link
Contributor Author

@janczer janczer commented Sep 21, 2018

I fixed the tests in the last commit. But I have to remove some rules about grid. Is it correct?

cc @ai @Dan503

@Dan503
Copy link
Contributor

@Dan503 Dan503 commented Sep 21, 2018

only by hands 😅

@ai how do you know what line number and character number the test is looking for then? Those are incredibly difficult to figure out manually.

Is it correct?

@janczer Not at all.

  1. You can't remove any of the auto stuff
  2. The align/justify-items warnings need to remain multi-line. It's too hard to read otherwise.
  3. Why did you remove the .warn_ie_align_content test?

For testing multi-line, you can replicate a new line by using \n.

This is line one
  This is line 2

=

"This is line one\n  This is line 2"

I recommend reverting that last commit completely and trying again.

@ai
Copy link
Member

@ai ai commented Sep 22, 2018

Sorry, I am drunk after a conference 😄😄😄

Will answer in next few days

@janczer
Copy link
Contributor Author

@janczer janczer commented Sep 22, 2018

@ai how do you know what line number and character number the test is looking for then? Those are incredibly difficult to figure out manually.

You can just run the test and check what exactly line has warning.

@janczer Not at all.

1. You can't remove any of the `auto` stuff

Ok, looks like it's problem with code that you added.

2. The `align/justify-items` warnings need to remain multi-line. It's too hard to read otherwise.

Autoprefixer has no warnings with new line. I think we can just change warning style.

3. Why did you remove the `.warn_ie_align_content` test?

Because here is a bug #1127.

I recommend reverting that last commit completely and trying again.

Looks like that problem with your commits, can you check it? Or I just revert it.

@janczer janczer force-pushed the janczer:add/wargnings-grid-ie branch from a353ef6 to 25ba8a2 Sep 22, 2018
@Dan503
Copy link
Contributor

@Dan503 Dan503 commented Sep 22, 2018

You can just run the test and check what exactly line has warning.

Ahh right I get it now, it refers to the line numbers in test\cases\grid.css

looks like it's problem with code that you added.

I think I might figured out what the main problem was but I'm still getting these weird test fails even though they look like they match up perfectly. Does anyone have any idea what is going on here?

non-sensical-test-fails

Autoprefixer has no warnings with new line.

There was a multi-line warning at one point 😕 I can't remember what the multi-line warning was for though. @ai @bogdan0083 Did you change it to a single line warning or remove it?

It would be a lot nicer if the warning could stay multi-line.

Because here is a bug

It looks like it's just a case of the rules being inconsistent. Since we write the test output manually I don't see why the rule can't be included. It just means that that particular rule has its properties the other way around.

When I tried to replicate it using the playground it didn't seem to have that issue. I needed to set the playground to "last 5 version" to get it to output the prefixes.

@bogdan0083
Copy link
Contributor

@bogdan0083 bogdan0083 commented Sep 22, 2018

I think I might figured out what the main problem was but I'm still getting these weird test fails even though they look like they match up perfectly. Does anyone have any idea what is going on here?

Try to remove the indentation and indent the lines by hand. It might be tabs/spaces problem 🤔.

There was a multi-line warning at one point 😕 I can't remember what the multi-line warning was for though. @ai @bogdan0083 Did you change it to a single line warning or remove it?

Yep, I removed it as we no longer needed warnings for duplicate area names. You can see an example of multiline warning here.

@Dan503
Copy link
Contributor

@Dan503 Dan503 commented Sep 22, 2018

Try to remove the indentation and indent the lines by hand. It might be tabs/spaces problem 🤔.

Nope :(

Yep, I removed it as we no longer needed warnings for multiple area names.

Ahh ok, yeah that makes sense.

@Dan503
Copy link
Contributor

@Dan503 Dan503 commented Sep 22, 2018

@bogdan0083 if you want to take a look at the error, you will find it on my feature/align-justify-grid-warnings branch.

@bogdan0083
Copy link
Contributor

@bogdan0083 bogdan0083 commented Sep 22, 2018

Thanks! Just wanted to ask 😀

@janczer
Copy link
Contributor Author

@janczer janczer commented Sep 22, 2018

@ai @bogdan0083 @Dan503 In the latest commit I fixed all test. Can we merge it now? Or I should change it?

@Dan503
Copy link
Contributor

@Dan503 Dan503 commented Sep 22, 2018

Well yeah... you fixed the errors but you did it by removing important functionality. You also got rid of the multi-line aspect. So no, we aren't merging yet.

I created a pull request against your branch with some updates but it has test errors in it. That's what we are figuring out now.

@janczer
Copy link
Contributor Author

@janczer janczer commented Sep 22, 2018

you fixed the errors but you did it by removing important functionality

What exactly functionality you mean? Can you please explain it here and I can add it.

I created a pull request against your branch with some updates but it has test errors in it.

I'm sorry. But you should fix your pull request.

@Dan503
Copy link
Contributor

@Dan503 Dan503 commented Sep 22, 2018

What exactly functionality you mean?

You removed all the warnings that Autoprefixer produces when people try to use grid auto properties like grid-auto-rows.

@bogdan0083
Copy link
Contributor

@bogdan0083 bogdan0083 commented Sep 22, 2018

Try to remove the indentation and indent the lines by hand. It might be tabs/spaces problem 🤔.
Nope :(

I've just checked your branch and I have no strange errors 🤔

@Dan503
Copy link
Contributor

@Dan503 Dan503 commented Sep 22, 2018

Really? so you run npm run test and it came back with zero errors?

@janczer
Copy link
Contributor Author

@janczer janczer commented Sep 22, 2018

You removed all the warnings that Autoprefixer produces when people try to use grid auto properties like grid-auto-rows.

It's not true. You can see it in the last commit: 25ba8a2

@Dan503
Copy link
Contributor

@Dan503 Dan503 commented Sep 22, 2018

Oh... sorry, yeah looks like it was altered rather than removed.

I still think the multi-line aspect is important though.

@bogdan0083
Copy link
Contributor

@bogdan0083 bogdan0083 commented Sep 22, 2018

Really? so you run npm run test and it came back with zero errors?

I have one error about the warning message, one about selector namings of this sort:

    - .warn_ei_align {
    + .warn_ie_align {

and one more:

      .warn_ie_align_content {
    +   -ms-flex-line-pack: center;
        -webkit-align-content: center;
    -   -ms-flex-line-pack: center;
        align-content: center;
        display: -ms-grid;
        display: grid;
      }

These are the only errors I get.

@janczer
Copy link
Contributor Author

@janczer janczer commented Sep 22, 2018

@bogdan0083 the second one related to #1127

@Dan503
Copy link
Contributor

@Dan503 Dan503 commented Sep 22, 2018

Ah ok, I'll fix up those 2 errors. My console is drowned in bogus errors for whatever reason so I couldn't see them.

@Dan503
Copy link
Contributor

@Dan503 Dan503 commented Sep 22, 2018

@bogdan0083 I've pushed theoretical fixes for those 2 issues. Can you test?

@bogdan0083
Copy link
Contributor

@bogdan0083 bogdan0083 commented Sep 22, 2018

Oh crap, I'm sorry. The first error is a little longer, I sent only a part of a diff to show you that my errors are different. Here's the full diff of 1 error:

    @@ -121,22 +121,28 @@
      /* Emit warning if grid enabled */
      .warn-display-contents .grid {
        display: contents;
      }

    - .warn_ei_align {
    + .warn_ie_align {
        -ms-flex-align: center;
            align-items: center;
        display: grid;
      }

    - .warn_ei_justify {
    + .warn_ie_justify {
        justify-items: center;
        display: grid;
      }

    - .warn_ei_justify_content {
    + .warn_ie_align_content {
    +   -ms-flex-line-pack: center;
    +       align-content: center;
    +   display: grid;
    + }
    +
    + .warn_ie_justify_content {
        -ms-flex-pack: center;
            justify-content: center;
        display: grid;
      }

And the error about the warning message:

@@ -14,16 +14,16 @@
        "autoprefixer: <css input>:102:3: grid-auto-rows is not supported by IE",
        "autoprefixer: <css input>:103:3: grid-auto-flow is not supported by IE",
        "autoprefixer: <css input>:104:33: auto-fill value is not supported by IE",
        "autoprefixer: <css input>:105:30: auto-fit value is not supported by IE",
        "autoprefixer: <css input>:121:3: Please do not use display: contents; if you have grid setting enabled",
    -   "autoprefixer: <css input>:125:3:
    +   "autoprefixer: <css input>:125:3:
        IE does not support align-items on grid containers.
        Try using align-self on child elements instead.
        Example: .grid > * { align-self: center }
      ",
    -   "autoprefixer: <css input>:130:3:
    +   "autoprefixer: <css input>:130:3:
        IE does not support justify-items on grid containers.
        Try using justify-self on child elements instead.
        Example: .grid > * { justify-self: center }
      ",
        "autoprefixer: <css input>:135:3: IE does not support align-content on grid containers.",

You will need to remove the trailing spaces on these lines

@bogdan0083
Copy link
Contributor

@bogdan0083 bogdan0083 commented Sep 22, 2018

If you want to omit those bogus errors you can try to run a single test by using jest -t 'test-name'

@Dan503
Copy link
Contributor

@Dan503 Dan503 commented Sep 22, 2018

Do you know what the test names go by?

@bogdan0083
Copy link
Contributor

@bogdan0083 bogdan0083 commented Sep 22, 2018

the first is

has disabled grid options by default

and the second is

supports grid layout
@bogdan0083
Copy link
Contributor

@bogdan0083 bogdan0083 commented Sep 22, 2018

Did you try to remove the fork and set it up again and then use some other editor? I don't know, something is breaking your tests 🤔

@Dan503
Copy link
Contributor

@Dan503 Dan503 commented Sep 22, 2018

There were other alterations that I made in my version as well in case people were thinking it's not worth the hassle.

  • I personalised the warning messages further so that it only mentions the property that the user used rather than all the properties.
  • I included place-items/content in the warning message
  • I moved the code into the area that the rest of the "grid-must-be-enabled" tests were in.
@Dan503
Copy link
Contributor

@Dan503 Dan503 commented Sep 22, 2018

If you want to omit those bogus errors you can try to run a single test by using jest -t 'test-name'

Using Jest directly worked 😊

Did you try to remove the fork and set it up again and then use some other editor?

umm... I didn't try that, I did try deleting node_modules and re-intalling though.

@bogdan0083
Copy link
Contributor

@bogdan0083 bogdan0083 commented Sep 22, 2018

Using Jest directly worked 😊

That's cool! Glad it helped.

@Dan503
Copy link
Contributor

@Dan503 Dan503 commented Sep 22, 2018

Ok I'll delete the align-content test. It is more difficult to solve that I was expecting 😕

@Dan503
Copy link
Contributor

@Dan503 Dan503 commented Sep 22, 2018

@janczer this is what I meant when I said it looked like you removed auto functionality

auto-stuff-removal

Found in this git commit: a353ef6

Or is there extra context around that which I'm missing? I don't see any replacement for that code in that commit.

@Dan503
Copy link
Contributor

@Dan503 Dan503 commented Sep 22, 2018

Our branches got out of sync and it was too difficult to merge so I just created a separate pull request instead #1129

@ai ai closed this in #1129 Sep 23, 2018
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.