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

keepWarnings option like in postcss-log-warnings #9

Open
gazay opened this issue May 11, 2015 · 4 comments
Open

keepWarnings option like in postcss-log-warnings #9

gazay opened this issue May 11, 2015 · 4 comments

Comments

@gazay
Copy link
Member

gazay commented May 11, 2015

I think by default we better not clear warnings after append them to result because there can be used other plugin after, like postcss-log-warnings. So option should be named as clearWarnings

@MoOx
Copy link
Contributor

MoOx commented May 11, 2015

+1.
I plan to integrate messages api in cssnext soon and I will really need to get both methods at the same time :)

@ai
Copy link
Member

ai commented May 11, 2015

I agree that clearWarnings option is better, because it is OK if we will show warning i browsers and CLI too.

lydell added a commit to lydell/postcss-log-warnings that referenced this issue May 29, 2015
Inspired by postcss-messages. Then why not use that plugin? Well, look at the
diff of index.js:

    -    console.log(warningLog);
    +    if (!('console' in options && !options.console)) {
    +      console.log(warningLog);
    +    }
    +
    +    if (options.browser) {
    +      result.root.append(formatBrowser(warningLog));
    +    }

The only difference is how to display the warnings! There's no need to duplicate
the rest of features between the two plugins.

This fixes postcss/postcss-browser-reporter#8, postcss/postcss-browser-reporter#9 and
postcss/postcss-browser-reporter#10.

As opposed to postcss-messages, this implementation:

- Inserts the messages at `head::before` instead of `html::before`. While
  `html::before` is unlikely to conflict with other styles, `head::before` is
  even less so! In fact, it should be _extremely_ unlikely to conflict.

- Does not allow to change the selector. Because of the above point there's no
  need to.

- Does not use `::after` if `::before` was already used in the CSS. Again,
  because of the first point there is no need to.

- Does not allow to change the styling. There's no way to change the styling
  when logging to the console either. Also, @ai said:

  > BTW, options is a lack in UX. You should better think more about default
  > options and styles, rather than add options for every case.

  postcss/postcss-browser-reporter#3 (comment)

- Uses simpler styling. KISS. It is still similar to postcss-messages.
lydell added a commit to lydell/postcss-log-warnings that referenced this issue May 30, 2015
Inspired by postcss-messages. Then why not use that plugin? Well, look at the
diff of index.js:

    -    console.log(warningLog);
    +    if (!('console' in options && !options.console)) {
    +      console.log(warningLog);
    +    }
    +
    +    if (options.browser) {
    +      result.root.append(formatBrowser(warningLog));
    +    }

The only difference is how to display the warnings! There's no need to duplicate
the rest of features between the two plugins.

This fixes postcss/postcss-browser-reporter#8, postcss/postcss-browser-reporter#9 and
postcss/postcss-browser-reporter#10.

As opposed to postcss-messages, this implementation:

- Inserts the messages at `head::before` instead of `html::before`. While
  `html::before` is unlikely to conflict with other styles, `head::before` is
  even less so! In fact, it should be _extremely_ unlikely to conflict.

- Does not allow to change the selector. Because of the above point there's no
  need to.

- Does not use `::after` if `::before` was already used in the CSS. Again,
  because of the first point there is no need to.

- Does not allow to change the styling. There's no way to change the styling
  when logging to the console either. Also, @ai said:

  > BTW, options is a lack in UX. You should better think more about default
  > options and styles, rather than add options for every case.

  postcss/postcss-browser-reporter#3 (comment)

- Uses simpler styling. KISS. It is still similar to postcss-messages.

- Properly escapes the CSS content string.
@MoOx
Copy link
Contributor

MoOx commented Jun 6, 2015

+1 for clearWarnings. Maybe @DavidClark could do the same for postcss-log-warnings.

@davidtheclark
Copy link

I'm fine with this. Created in issue in log-warnings to get it done.

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

No branches or pull requests

4 participants