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

Fixed responsive breakout points #1077

Conversation

robert9g
Copy link
Contributor

Fixes #1029 .

Changes:

  • decreased max-width media queries breakpoints by one pixel

Doing this avoids the issues of conflicting breakpoints at 768px and 998px, these breakpoints were present in both:

  • the min-width media queries, namely @small and @medium
  • and the max-width media queries, namely @small-max-width and @medium-max-width

@bonustrack bonustrack temporarily deployed to busy-nd-pr-1077 November 16, 2017 15:16 Inactive
@Sekhmet
Copy link
Contributor

Sekhmet commented Nov 16, 2017

I think that we should get rid of half of our breakpoints. We currently have breakpoints for min-width and max-width as well. Some components use the former, other the latter.
I think that we should decide on one way of doing this, and update styles to use one of them.
I created min-width first, but I don't think it's the best way of doing this. I was looking for "proper way" of doing this, but both were used.
I don't have a strong opinion on this, so I will let you decide on this.
cc: @sirrius-a-b @jm90m

@jm90m
Copy link
Contributor

jm90m commented Nov 17, 2017

I have no strong opinion on this either, this doesn't seem be a real issue, both min & max-width are fine to be used. In some websites, there are more than 2 transition states, it looks like for us there is 3... I think it's fine, since no styling gets messed up

@robert9g
Copy link
Contributor Author

I think is quite all right to have 3 responsive states given the current layout. Many sites use bootstrap that has 4 states (xs, sm, md, lg).

I did a few searches and it seems that the @small ( used in 14 files) and @medium (used in 4 files) vars are used in more files that the @small-max-width (used in 4 files) and @medium-max-width (used in 3 files). This match numbers exclude the variables.less file in which they were declared.

I do feel however that the @small and @medium vars do not properly describe their function. For example, reading the code below, one might understand that somediv has it's color set to red, but on small screens it is white. But with how the vars are set, the opposite is true.

.somediv {
color: red;
@small {
color: white;
}
}

It would make more sense in the above example if instead of small name it would be medium name. And the logic would be somediv has it's color red but starting with medium it will be white.

For quick reference, the current media vars:
responsive-vars

Anyway, I agree that there should probably be just one set of media vars.

What do you think of the idea of:

  • change all @medium var name to @large
  • change all @small var name to @medium
  • replace @small-max-width and @medium-max-width and the related code to use the newly created vars above.

This way the logic of the CSS would remain as it is :'responsive first', and also no code relating @small and @medium would change apart from the actual name of the vars.

So in the end we will have just 2 vars: @medium and @large.

Sorry for the wall of text, let me know how you feel about this when you get a chance or if you have questions.

@robert9g
Copy link
Contributor Author

robert9g commented Nov 21, 2017

Did some of you got a chance to read my suggestions above? Should I start the conversion of the @media vars? If not, are there any suggestion about this PR? Because I think that it satisfies #1029.

@jm90m
Copy link
Contributor

jm90m commented Nov 21, 2017

@sirrius-a-b hey sry, we've been busy, but in my opinion I kinda like the @small-max-width variable names... but it could be my personal preference. I feel like it provides more context to the user when you write styles.

But yeah I have no preference like I said before. You can start the conversion of the files, to @Medium, @Small, etc.. and replace @small-max-width... but it seems like its a big conversion and messing up on this can cause a few issues. oh well @bonustrack can have fun testing this one out.

@bonustrack bonustrack requested a deployment to busy-nd-pr-1077 November 21, 2017 15:41 Pending
@bonustrack bonustrack requested a deployment to busy-nd-pr-1077 November 21, 2017 15:41 Pending
@bonustrack bonustrack temporarily deployed to busy-nd-pr-1077 November 21, 2017 18:30 Inactive
@bonustrack bonustrack temporarily deployed to busy-nd-pr-1077 November 21, 2017 18:30 Inactive
@bonustrack bonustrack temporarily deployed to busy-master-pr-1077 November 22, 2017 12:38 Inactive
@bonustrack bonustrack temporarily deployed to busy-master-pr-1077 November 22, 2017 12:39 Inactive
@bonustrack bonustrack temporarily deployed to busy-master-pr-1077 November 23, 2017 21:02 Inactive
@robert9g
Copy link
Contributor Author

I removed @small-max-width and @medium-max-width media vars and modified all relevant LESS code to only use @small and @medium.

When you get a change please take a look, test it. I haven't found any issues myself. Maybe see if the merges above are all right too.

Thanks.

@jm90m jm90m changed the base branch from new-design to master November 23, 2017 22:19
@bonustrack
Copy link
Contributor

Depending on the width of the window i may see the interface like this:
image
I think there should be a margin in the red lines, the content should not start in left and right without a default margin.

@robert9g
Copy link
Contributor Author

You are right @bonustrack that happens between 998px and ~1030px. I will fix that.

I did not made any changes to the layout before on purpose because I just wanted to remove the 2 extra media vars as suggested above.

@bonustrack bonustrack temporarily deployed to busy-master-pr-1077 November 25, 2017 00:54 Inactive
Copy link
Contributor

@bonustrack bonustrack left a comment

Choose a reason for hiding this comment

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

That's looking perfect to me @sirrius-a-b 🥇 can you resolve conflicts?

@bonustrack bonustrack temporarily deployed to busy-master-pr-1077 November 27, 2017 22:31 Inactive
@robert9g
Copy link
Contributor Author

Thanks a lot. I resolved the conflicts, but please test again before merging :)

@bonustrack bonustrack temporarily deployed to busy-master-pr-1077 November 29, 2017 12:48 Inactive
@bonustrack bonustrack temporarily deployed to busy-master-pr-1077 November 30, 2017 02:06 Inactive
@bonustrack bonustrack temporarily deployed to busy-master-pr-1077 November 30, 2017 02:10 Inactive
@bonustrack bonustrack temporarily deployed to busy-master-pr-1077 November 30, 2017 23:39 Inactive
@bonustrack bonustrack merged commit 70d735c into busyorg:master Dec 1, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants