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

Allow to display warnings in the browser #7

Closed
wants to merge 1 commit into from

Conversation

lydell
Copy link
Contributor

@lydell lydell commented 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.

    Add option to customize styles of the message box postcss/postcss-browser-reporter#3 (comment)

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

  • Properly escapes the CSS content string.

@ai
Copy link

ai commented May 29, 2015

What is a point to have one huge plugin instead of two “one task” plugins? For example, right now we can display some warnings in console and some in browser.

I think postcss-messages and postcss-log-warning have too different behaviour to be implemented in one plugin.

@lydell
Copy link
Contributor Author

lydell commented May 29, 2015

Really, do you think this plugin is huge? This PR does not even add 100 lines of code (not counting test code).

Benefits from merging:

@ai
Copy link

ai commented May 29, 2015

  1. PR to fix that problems will be smaller ;).
  2. It will be better to create a common library to work with warnings.
  3. We have more difference, because users will use this plugin for different task. How throwError optioin will work in browser mode? Or there is no sense to change console warning styles, but there is sense to change browser style. And wwe again will have a option only for one mode.

I think it is like a megre Autoprefixer and Autopolyfiller. Of course, there is a common code. Of course that do something similiar. But it is look like platypus. Plugin will have more complicated UX, because he will have different modes, when some options will work and other will not.

@lydell
Copy link
Contributor Author

lydell commented May 30, 2015

PR to fix that problems will be smaller ;)

I don’t think so. Prove me wrong ;)

It will be better to create a common library to work with warnings.

postcss-helper-create-warnings-string?

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.
@lydell
Copy link
Contributor Author

lydell commented May 30, 2015

@MoOx, you usually have good opinions on PostCSS stuff. What do you think?

@lydell
Copy link
Contributor Author

lydell commented May 30, 2015

Sometimes you need a good night’s sleep. I just woke up with the answer. I will now start to break this PR up, and post new PRs, some to this module, some to postcss-messages.

@lydell lydell closed this May 30, 2015
lydell added a commit to lydell/postcss-log-warnings that referenced this pull request May 30, 2015
I updated it by modifying and running test/visual.js. This makes it really easy
to update it in the future: Just run `npm run visual` and grab a screenshot of
the result!

The above also simplifies the code of test/visual.js and fixes a bug where the
thrown error wasn't shown in the console.

This commit is split out of davidtheclark#7.
@lydell lydell mentioned this pull request May 30, 2015
lydell added a commit to lydell/postcss-log-warnings that referenced this pull request May 30, 2015
This allows another plugin, such as postcss-messages, to use
postcss-log-warnings as a library and display the warnings in a different way.

See davidtheclark#7.
lydell added a commit to lydell/postcss-log-warnings that referenced this pull request May 30, 2015
This allows another plugin, such as postcss-messages, to use
postcss-log-warnings as a library and display the warnings in a different way.

See davidtheclark#7 and davidtheclark#9.
@davidtheclark
Copy link
Owner

I've got to catch up on this conversation ...

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

Successfully merging this pull request may close these issues.

Unify text format
3 participants