Skip to content

Conversation

@SibrenVasse
Copy link

@SibrenVasse SibrenVasse commented Feb 24, 2019

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

@jelly
Copy link
Member

jelly commented Feb 24, 2019

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" %}"
Copy link
Member

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

Copy link
Author

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.

Copy link
Member

@jelly jelly left a 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.

@SibrenVasse
Copy link
Author

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! :(

@jelly
Copy link
Member

jelly commented Feb 24, 2019

Thanks! Now it takes less time to review the whole PR 👍

@Bluewind
Copy link
Contributor

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 .table-wrapper divs all over the place. This could be pulled out into a commit that performs only this change, including the indentation changes that follow due to the new div, but that does nothing else. Other wrappers like .top-wrapper would also get similar dedicated commits. The removal of class="{% cycle 'odd' 'even' %}" can also be done in a dedicated commit together with the respective change to the css. Cleanup like the removal of <label></label> should also get a dedicated commit. Adding the RSS icon should also be a dedicated commit. I'm not sure which changes remain after all this is done, but you probably understand what my goal here is.

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:

  • Start with git reset HEAD~ to remove the big commit.
  • Next use git add -pi and only add changes that are part of a single logical change like, for example, .table-wrapper. If the automatic chunk detection is not enough because you have multiple changes in the same line or close together, press e during the add -pi operation to edit the diff. Change the diff such that it only adds the particular change you want to add.
  • Once all partial changes are added, check with git diff --cached --stat -p if the diff really only contains changes that are a single logical change. If there are any other changes use git reset -p to remove them from the staging area again and add the correct things with git add -pi.
  • When the diff is correct, commit as usual and repeat the steps until all changes are committed.

If you later find that you need to edit a particular commit use git commit --fixup $id to link the changes to the original commit. When you are done run git rebase --autosquash -i $base_commit to squash them. Alternatively just commit the fix in a dedicated, new commit and keep the history as it is. What you want to do depends on the particular problem and how the changes are merged.

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 .table-wrapper and the remaining responsiveness changes.

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!

@jelly jelly mentioned this pull request Apr 14, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants