Skip to content
This repository was archived by the owner on May 4, 2020. It is now read-only.

Add the possibility to use media function for height based media query#17

Merged
lusimeon merged 2 commits intodevelopfrom
feature/height-media-query
Apr 1, 2019
Merged

Add the possibility to use media function for height based media query#17
lusimeon merged 2 commits intodevelopfrom
feature/height-media-query

Conversation

@lusimeon
Copy link
Copy Markdown
Contributor

Proposal for #4.

@lusimeon lusimeon added the enhancement New feature or request label Mar 25, 2019
Comment thread src/framework/_breakpoints.scss
@perruche perruche self-requested a review March 27, 2019 10:39
Copy link
Copy Markdown
Contributor

@perruche perruche left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@titouanmathis
Copy link
Copy Markdown
Contributor

Nice!
Maybe it would be simpler to keep the width and height definitions in one map ?

$breakpoints: (
  height: (
    xxs: 0,
    //...
  ),
  width: (
    xxs: 0,
    xs: 480,
    // ...
  ),
);

@titouanmathis titouanmathis added this to the 1.2.0 milestone Mar 28, 2019
@titouanmathis
Copy link
Copy Markdown
Contributor

Will close #4 on merge 👍

@perruche
Copy link
Copy Markdown
Contributor

I'am not sure it's a good idea to use the same map for height and width:

  • we will have to rewrite all the breakpoints based utility classes.
  • We can't use the same keys as in your example .type-center--s would be a height or width based class ?
  • I don't think height based utility classes are really useful.

What do you think ?

@titouanmathis
Copy link
Copy Markdown
Contributor

It all makes sense, all good for me! 👍

Copy link
Copy Markdown
Contributor

@titouanmathis titouanmathis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@lusimeon you should update the documentation in the readme too

@lusimeon lusimeon force-pushed the feature/height-media-query branch from 644df0d to a24d187 Compare March 29, 2019 11:27
@lusimeon lusimeon requested a review from titouanmathis March 29, 2019 11:28
Comment thread readme.md
@lusimeon lusimeon requested a review from titouanmathis March 29, 2019 13:02
@titouanmathis
Copy link
Copy Markdown
Contributor

@lusimeon you can merge your feature 😉

Note: maybe you should do it directly on Github with the big green button, it seems like closing a feature with Git Flow marks the PR as closed and not merged :/

@lusimeon lusimeon merged commit 74fcf56 into develop Apr 1, 2019
@lusimeon lusimeon deleted the feature/height-media-query branch April 1, 2019 09:05
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants