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 option to customize styles of the message box #3

Closed
MoOx opened this issue Mar 30, 2015 · 6 comments
Closed

Add option to customize styles of the message box #3

MoOx opened this issue Mar 30, 2015 · 6 comments

Comments

@MoOx
Copy link
Contributor

MoOx commented Mar 30, 2015

No description provided.

@gazay
Copy link
Member

gazay commented Apr 3, 2015

I've added it here 9fb2184, but I'm not a JS programmer and could mess with code style, can you please help and review it? Also ci is failing after my changes: https://travis-ci.org/postcss/postcss-messages/jobs/56995895.

/cc @ai, @MoOx

@gazay
Copy link
Member

gazay commented Apr 3, 2015

CI is not failing anymore, I've figured our that there were unnecessary semicolons.

@ai
Copy link
Member

ai commented Apr 3, 2015

styleKeys is not a good solution. User may want to se some new style. It is better to add style key in options:

messages({
  style: {
    'box-shadow': '0 0 10px black'
  }
});

@ai
Copy link
Member

ai commented Apr 3, 2015

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

@gazay
Copy link
Member

gazay commented Apr 3, 2015

styleKeys here is excess code, firstly I thought to do it this way, but then I just set default styles and give users way to override or add new styles

@gazay
Copy link
Member

gazay commented Apr 3, 2015

Done

@gazay gazay closed this as completed Apr 3, 2015
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.
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

3 participants