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

Discussion for v3 API for more flexibility and readability #11

Open
oyeanuj opened this issue Nov 7, 2017 · 15 comments
Open

Discussion for v3 API for more flexibility and readability #11

oyeanuj opened this issue Nov 7, 2017 · 15 comments

Comments

@oyeanuj
Copy link

oyeanuj commented Nov 7, 2017

Hi @morajabi, thank you for putting out this library. It has been super helpful so far. In my current project, I keep needing to write height based media queries, and in doing so, I have to opt traditional media query rather than use this library.

So, I was wondering if you are considering adding support for other aspects of media query in the library API?

@morajabi
Copy link
Owner

morajabi commented Nov 7, 2017

@oyeanuj Thanks! I had something similar in mind for a while now. There's a couple of ways to do this. One possible implementation is to have 2 top level keys on the breakpoints object:

const breakpoints = {
  width: {
    large: '1170px',
    medium: '768px',
    small: '450px',
  },
  height: {
    small: '700px',
  }
}

Then probably we can change the API like this:

const Box = styled.div`
  background: black;

  ${media.width.lessThan('medium')`
    background: red;
  `}
`

Although it's a rough idea. I'm afraid I don't have time currently (maybe after this week?) I'll probably want to talk to SC teammates to refine the API to be more idiomatic to styled components for version 3.

@oyeanuj
Copy link
Author

oyeanuj commented Nov 7, 2017

@morajabi Thanks for the quick response. Your proposal looks good! I was thinking along the same lines for the API with additional support for:

media.orientation.is('landscape')
media.orientation.not('portrait')
media.screen

A bonus API feature could be if they could be chained like below:

media.width.lessThan('medium').orientation('landscape')

Thoughts?

@morajabi
Copy link
Owner

morajabi commented Nov 7, 2017

@oyeanuj That would add a lot of additional API complexity which I don't want this to be harder than the default CSS media query syntax. BTW for your problem a quick fix might be to export your breakpoint variables and just use them with interpolation in SC styles:

import styled from 'styled-components'
import { breakpoints } from '../utils/media'

const Wrapper = styled.div`
  @media landscape and (min-width: ${breakpoints.medium}) {
    /* whatever */
  }
`

@tlvenn
Copy link

tlvenn commented Mar 12, 2018

Chainable API is pretty elegant and imho does not increase the complexity. If you dont need to specify the orientation, it changes nothing and if you do, you only have to chain another constraint, you dont have to modify your current constraint to accommodate it.

@morajabi
Copy link
Owner

morajabi commented Mar 12, 2018

What if we stick to the syntax that we have but let the library write the most confusing part which is min-with, max-width as well as for height?

@media landscape and ${media.width.lessThan('small')} {
}

and this one for height:

@media landscape and ${media.height.between('small', 'big')} {
}

Now we are not reinventing the wheel. Thus we now have a very flexible API and syntax highlighting which I believe we don't have now because we're not using css` `

Basically, the @media syntax is fine in most situations but specifying sizes is very confusing, I'd like to just solve the pain point and not reinvent a whole new media query syntax™ 😉

The only problem is it's a bit lengthy, and I don't like lengthy media queries, I often wrap them in a function to use them like so:

${mobile(css`
 /* -- */
`)}

Thoughts about the API and if you have suggestions to make it less lengthy? @oyeanuj @ApacheEx @tlvenn

@ApacheEx
Copy link
Collaborator

ApacheEx commented Mar 12, 2018

here is all media features: https://www.w3.org/TR/css3-mediaqueries/#media1
currently, we support only min-width, max-width 💪

should we support all media features or (min/max) width, (min/max) height, orientation would be enough - that's also a question to discuss.

I don't like lengthy media queries

💯 agree

p.s. I will try to make some suggestions a bit later 🤔

@morajabi
Copy link
Owner

morajabi commented Mar 12, 2018

should we support all media features or width, height, orientation would be enough - that's also a question to discuss.

That's exactly why I'm saying going this route might not be the best way to do it, replicating everything from the original API doesn't seem good.

@ApacheEx What do you think about that API I suggested?

@ApacheEx
Copy link
Collaborator

ApacheEx commented Mar 12, 2018

ok,

  1. yours: (looks good but a bit lengthy)
const Button = styled.button`
  width: 120px;
  @media (orientation: landscape) and ${media.width.between('small', 'big')} {
     width: 300px;
  }
`;
  1. mine: (a bit complex, but quite flexible)

Idea

Add features possibility which supports any features from https://www.w3.org/TR/css3-mediaqueries/#media1. What you put there is what you get in media (no extra logic).

I love current API, let's make something similar ❤️

const defaultFeatures = {
  landscape: {
    orientation: 'landscape',
  },
  portrait: {
    orientation: 'portrait',
  },
}

If I want to add my own features:

// ./src/styles/style-utils.js

// My own breakpoints.
const breakpoints = {
  xs: '250px',
  sm: '450px',
};

// My own features.
const features = {
  iphoneX: {
    orientation: 'landscape',
    'device-aspect-ratio': '21/9',
    ...etc 
  },
  ..etc
};

export const media = generateMedia(breakpoints, features);

Finally:

// ./src/components/Button.js

import { media } from 'styles/style-utils';

const Button = styled.button`
  width: 120px;
  ${media.width.between('xs', 'sm')('iphoneX')`
     width: 300px;
  `};
  ${media.width.greaterThan('sm')('landscape')`
     width: 400px;
  `};
  ${media.greaterThan('sm')`
     width: 400px;
  `};
`;

will generate

@media all and (min-width: 250px) and (max-width: 450px) and (orientation: landscape) and (device-aspect-ratio: 21/9) { … }

@media all and (min-width: 450px) and (orientation: landscape) { … }

@media all and (min-width: 450px) { … }

Thoughts?

@morajabi
Copy link
Owner

morajabi commented Mar 13, 2018

I really love the API design of features. 💯 @ApacheEx

I'd like to add one more thing named aliases:

const aliases = media => {
  mobile: media.width.lessThan('sm'),
  tablet: media.width.between('sm', 'lg'),
}

export const media(breakpoints, features, aliases)

Then you can use the old API as well as the new features API AND the shorthands/aliases you declared:

const Wrapper = styled.div`
   ${media.mobile``}
   ${media.for('retina')``}
   ${media.width.lessThan('small').for('retina')``}
`

Sometimes I only want a really custom media query and I only use it once, it doesn't make sense to make a feature for everything, to solve this, we can get the media string by calling it without argument:

const Wrapper = styled.div`
  @media landscape and ${media.mobile} {
  }
`

All of these are quite optional for user to provide.

I'm was a fan of V1 API because it was so minimal like so: media.mobile, I'm trying to achieve that simplicity again.

@ApacheEx
Copy link
Collaborator

ApacheEx commented Mar 13, 2018

wow, looks awesome 💪 🔥

so, as conclusion:

  1. add features which can be applied via for:
    ${media.for('retina')`
       display: flex;
    `};
    
    ${media.width.between('sm', 'lg').for('retina')`
       display: block;
    `};
  2. add aliases which simplify life with lengthy/complex queries:
    ${media.mobile`
       display: flex;
    `};
    
    ${media.tablet.for('retina')`
       display: block;
    `};
  3. add possibility to use inline media:
    @media print and ${media.mobile} {
      display: flex;
    }
    
    @media all and ${media.width.lessThan('small')} {
      display: block;
    }

agree? am I miss something?

@oyeanuj
Copy link
Author

oyeanuj commented Mar 25, 2018

@ApacheEx Looks good, and the readability is helped with the aliases and chaining. Looking forward to this!

@morajabi
Copy link
Owner

Looks awesome @ApacheEx!

What do you think about having syntax highlighting? Currently, anything tagged with css`` gets CSS syntax highlighting, but as we don't use that currently we are missing the beautiful syntax highlighting in editors and IDEs.

/cc @julienp wdyt?

@morajabi morajabi changed the title Support for height and orientation based media queries Discussion for v3 API for more flexibility and readability Mar 26, 2018
@ghost
Copy link

ghost commented Oct 24, 2019

Is version 3 under development? I hope so, because the API for version 3 looks excellent and it would be a shame if it doesn't get implemented.

@timhagn
Copy link
Collaborator

timhagn commented Oct 26, 2019

Hi @ALL,

as this issue is open long enough and, as seen with #17, some things gotta change, I just proposed a backwards compatible way of going at the really excellent looking API for V3:
Chainable Callable Class Functions : ).

Have a look at #20 and tell me what you think, should you be so kind.

Best,

Tim.

@oyeanuj
Copy link
Author

oyeanuj commented Feb 18, 2020

@morajabi Is v3 still on the cards? If so, any preferences between @ApacheEx and @timhagn's proposals?

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

No branches or pull requests

5 participants