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

"extensions" option lost since new versions? #43

Closed
kud opened this Issue Oct 8, 2018 · 7 comments

Comments

Projects
None yet
2 participants
@kud

kud commented Oct 8, 2018

I don't see in the releases the fact you removed the option/attribute "extensions" which I used.

https://github.com/postcss/postcss-custom-media/releases

@kud

This comment has been minimized.

Show comment
Hide comment
@kud

kud Oct 8, 2018

By the way, I've tried different styles of import and the only way to make it work is:

const customMedia = require("./mediaQueries.json")

require("postcss-custom-media")({
    importFrom: [{ customMedia }],
}),

require("postcss-custom-media")({
    importFrom: "./mediaQueries.json",
}),

didn't work for instance.

kud commented Oct 8, 2018

By the way, I've tried different styles of import and the only way to make it work is:

const customMedia = require("./mediaQueries.json")

require("postcss-custom-media")({
    importFrom: [{ customMedia }],
}),

require("postcss-custom-media")({
    importFrom: "./mediaQueries.json",
}),

didn't work for instance.

@jonathantneal

This comment has been minimized.

Show comment
Hide comment
@jonathantneal

jonathantneal Oct 8, 2018

Member

I’d have to see your mediaQueries.json file to know why. I’m guessing that it doesn’t follow the naming pattern.

JavaScript files, JSON files, functions, and objects will need to namespace custom media using the customMedia or custom-media key.
https://github.com/postcss/postcss-custom-media#importfrom

See the examples in that section for further clarification.

Member

jonathantneal commented Oct 8, 2018

I’d have to see your mediaQueries.json file to know why. I’m guessing that it doesn’t follow the naming pattern.

JavaScript files, JSON files, functions, and objects will need to namespace custom media using the customMedia or custom-media key.
https://github.com/postcss/postcss-custom-media#importfrom

See the examples in that section for further clarification.

@kud

This comment has been minimized.

Show comment
Hide comment
@kud

kud Oct 8, 2018

{
  "--mobile": "(max-width: 480px)",
  "--mobile-only": "(max-width: 480px)",
  "--mobile-excluded": "(min-width: 481px)",
  "--tablet": "(max-width: 1024px)",
  "--tablet-only": "(min-width: 481px) and (max-width: 1024px)",
  "--desktop": "(min-width: 1025px)",
  "--desktop-only": "(min-width: 1025px)"
}

I use it like this because I've got a component called <MediaQuery which="tablet" /> which helps me to handle responsive design with the same definitions that I've got for my CSS (via your plugin), so I can't change it like this the pattern of my custom media queries for the moment. But I could adjust, yeah.

// import styles from "./index.module.css"

import React from "react"
import PropTypes from "prop-types"
import ReactResponsive from "react-responsive"

import { MQ } from "~/constants"

export const isMediaMatched = prop => {
  const query = MQ[`--${prop}`] ? MQ[`--${prop}`] : prop

  return window.matchMedia(query).matches
}

class MediaQuery extends React.PureComponent {
  static propTypes = {
    children: PropTypes.node.isRequired,
    which: PropTypes.string,
  }

  static defaultProps = {
    which: null,
  }

  render() {
    const { which, children, ...rest } = this.props
    const query = which ? MQ[`--${which}`] : null

    return (
      <ReactResponsive query={query} {...rest}>
        {children}
      </ReactResponsive>
    )
  }
}

export default MediaQuery

kud commented Oct 8, 2018

{
  "--mobile": "(max-width: 480px)",
  "--mobile-only": "(max-width: 480px)",
  "--mobile-excluded": "(min-width: 481px)",
  "--tablet": "(max-width: 1024px)",
  "--tablet-only": "(min-width: 481px) and (max-width: 1024px)",
  "--desktop": "(min-width: 1025px)",
  "--desktop-only": "(min-width: 1025px)"
}

I use it like this because I've got a component called <MediaQuery which="tablet" /> which helps me to handle responsive design with the same definitions that I've got for my CSS (via your plugin), so I can't change it like this the pattern of my custom media queries for the moment. But I could adjust, yeah.

// import styles from "./index.module.css"

import React from "react"
import PropTypes from "prop-types"
import ReactResponsive from "react-responsive"

import { MQ } from "~/constants"

export const isMediaMatched = prop => {
  const query = MQ[`--${prop}`] ? MQ[`--${prop}`] : prop

  return window.matchMedia(query).matches
}

class MediaQuery extends React.PureComponent {
  static propTypes = {
    children: PropTypes.node.isRequired,
    which: PropTypes.string,
  }

  static defaultProps = {
    which: null,
  }

  render() {
    const { which, children, ...rest } = this.props
    const query = which ? MQ[`--${which}`] : null

    return (
      <ReactResponsive query={query} {...rest}>
        {children}
      </ReactResponsive>
    )
  }
}

export default MediaQuery
@jonathantneal

This comment has been minimized.

Show comment
Hide comment
@jonathantneal

jonathantneal Oct 8, 2018

Member

That first example looks good, as long as you are passing into a customMedia or custom-media key on the object being passed into importFrom.

If you’re still having issues, could you setup a small project replicating the issue so that I could take a look?

Here is a test we have for this functionality https://github.com/postcss/postcss-custom-media/blob/master/.tape.js#L15-L20

Member

jonathantneal commented Oct 8, 2018

That first example looks good, as long as you are passing into a customMedia or custom-media key on the object being passed into importFrom.

If you’re still having issues, could you setup a small project replicating the issue so that I could take a look?

Here is a test we have for this functionality https://github.com/postcss/postcss-custom-media/blob/master/.tape.js#L15-L20

@kud

This comment has been minimized.

Show comment
Hide comment
@kud

kud Oct 8, 2018

It's ok, it works with

const customMedia = require("./mediaQueries.json")

require("postcss-custom-media")({
    importFrom: [{ customMedia }],
}),

My main issue is that on release tab, we don't see you removed "extensions" attribute, and I had no error when compiling. But when I see my website, --tablet wasn't understood and the css rendered badly.

Two ways of improvement, when changing the API:

  • Write in the release/CHANGELOG.md that an option/attribute is deleted
  • Show a warning during the compilation (for like 1 version , like you can have in React) that extensions is not more understood.

Because I had the bad surprise to see that the compilation ran well but the render wasn't good.

kud commented Oct 8, 2018

It's ok, it works with

const customMedia = require("./mediaQueries.json")

require("postcss-custom-media")({
    importFrom: [{ customMedia }],
}),

My main issue is that on release tab, we don't see you removed "extensions" attribute, and I had no error when compiling. But when I see my website, --tablet wasn't understood and the css rendered badly.

Two ways of improvement, when changing the API:

  • Write in the release/CHANGELOG.md that an option/attribute is deleted
  • Show a warning during the compilation (for like 1 version , like you can have in React) that extensions is not more understood.

Because I had the bad surprise to see that the compilation ran well but the render wasn't good.

@jonathantneal

This comment has been minimized.

Show comment
Hide comment
@jonathantneal

jonathantneal Oct 8, 2018

Member

I’m sorry about that. The API changes were mine.

This plugin broke on minimally complex media queries before v7, and there was no one to update the plugin with me, so I re-wrote it from scratch, along with at least two other plugins. One of the casualties was not understanding and documenting the previous API in some cases, like this one.

I hope future migrations are easier for you, because Custom Media, Custom Selectors, and Custom Properties, and a handful of other plugins now all share the same importFrom API moving forward.

Member

jonathantneal commented Oct 8, 2018

I’m sorry about that. The API changes were mine.

This plugin broke on minimally complex media queries before v7, and there was no one to update the plugin with me, so I re-wrote it from scratch, along with at least two other plugins. One of the casualties was not understanding and documenting the previous API in some cases, like this one.

I hope future migrations are easier for you, because Custom Media, Custom Selectors, and Custom Properties, and a handful of other plugins now all share the same importFrom API moving forward.

@kud

This comment has been minimized.

Show comment
Hide comment
@kud

kud Oct 8, 2018

No worries @jonathantneal. Thank you a lot for your work :) Really appreciate it.

And you're right, "extensions" wasn't so meaningful.

But you express a breaking change anyway by doing a 7.0.0 version. So maybe I should be less lazy and read better the doc :p

kud commented Oct 8, 2018

No worries @jonathantneal. Thank you a lot for your work :) Really appreciate it.

And you're right, "extensions" wasn't so meaningful.

But you express a breaking change anyway by doing a 7.0.0 version. So maybe I should be less lazy and read better the doc :p

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment