-
-
Notifications
You must be signed in to change notification settings - Fork 144
[RFC] Implement responsive layout #203
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
Conversation
|
Cool, I'll try it out later this week and we can merge it in stages. |
|
|
||
| <a href="https://icons8.com/" title="Icons8"> | ||
| <img width="200" height="200" src="{% static "icons8_logo.png" %}" | ||
| <img src="{% static "icons8_logo.png" %}" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All style changes, please make a separate commit / PR for them
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The specified width and height on these images breaks on small screen widths.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This changes too much, re-indents, removal of id's and text/javascript changes and more style changes which don't have anything to do with adding responsive layout to archweb.
|
I'm sorry I had included a little bit too much. It's a fine line between cleaning up the html to be able to do something reponsive and something more. I hope this is better. I only wanted to help! :( |
|
Thanks! Now it takes less time to review the whole PR 👍 |
|
The changes would be much easier to review if you could split them into multiple smaller commits. One thing that jumps out is the addition of the I fully agree that splitting commits like this is quite some work, but I'd argue that properly reviewing the changes is not possible otherwise because there are just too many things to keep in your head at once. If you want to read the thoughts of others on this matter I recommend this OpenStack page. As for how to split commits like this I suggest the following work flow:
If you later find that you need to edit a particular commit use As for the order in which you commit changes: In theory, some changes may be merged before the rest, but this requires that they can stand on their own. You should strive to make this possible if only for the reason that you no longer have to deal with the change once it is merged. This means that you should first commit uncontroversial/easy/safe changes and only then deal with the potentially difficult/controversial/nasty stuff. In this case, I'd suggest to perform the various cleanups of unused classes/elements first, followed by the simplification of the even/odd handling and finally deal with the addition of the Having said all that, I'm not, strictly speaking, an archweb developer and it is highly unlikely that I would merge any patches here. Others may chose to merge this PR as it is, but if you find that reviews are going slow, this complexity may be the reason and the way I propose might be a solution. And finally, the video looks great and I'd love to see the changes being merged. Great work so far! |
Sorry for the large PR, but I saw no easy way to split it up.
Used a few new css features, most notably:
Tested on Firefox and Chrome on desktop and mobile.
Quick preview: https://files.sibrenvasse.nl/gjloNKWAN96ftqnv5q9PVOY8gpr9Xd/archweb.mp4