search

wics-uw / website

The code that runs http://wics.uwaterloo.ca/
Other
9 stars 49 forks source link

adjusted css for consistency of spacing #115

Closed mxyang42 closed 8 years ago

mxyang42 commented 8 years ago

I don't know how to access to @annalorimer 's local repository to rebase her commits, so I made my own changes. Let me know if I should fix anything. Thanks!

-MX

ehashman commented 8 years ago

In order to fetch commits from another person's repository, you need to do the following:

git add remote repo_name <git URL you can get from visiting their personal repository>
git fetch repo_name branch_name
git checkout repo_name/branch_name

For example,

git add remote ehashman git@github.com:ehashman/website-wics.git
git fetch ehashman sarah_transcript
git checkout ehashman/sarah_transcript

Once you've fetched the remote repository you can easily cherry pick commits, rebase, etc. Checkout not strictly necessary as it will put you into detached HEAD state.

I'm going to deploy the site for reference (see above) but I can't merge this PR in its current state. Two major issues:

  1. All the CSS changes should be in a separate commit, but they are lumped in with the content changes.
  2. You made hundreds of whitespace changes in the CSS which makes it impossible to see what you changed at a glance. That also needs a separate commit, if it's necessary at all. (I'd argue it isn't, but your editor probably did it automatically. You should probably turn that off.)
mxyang42 commented 8 years ago

Thanks Elana for the explanation on fetching commits from another person's repository. I thought I'd have to have permission for that! I agree with the bold font, smaller header, and that the css changes should be separated. Though I do think it's better to reformat the css file afterwards as well, just pretty code is more pleasant to read.

mxyang42 commented 8 years ago

@ehashman I updated stuff, how does it look now?

evykassirer commented 8 years ago

@ehashman did you want to look this over more before it gets merged?

ehashman commented 8 years ago

I am comfortable with merging the content commit but I am unsure about the CSS/whitespace commit. Hence, I am going to manually merge your first commit and leave the second one for later. Will leave this pull request open because a) the review app isn't automatically rebuilding when you're pushing new commits so I'd like to attempt to debug that with Heroku and b) that way I won't forget about the second commit later.

ehashman commented 8 years ago

@leafa: perhaps you should rebase this with master? Or close this PR, depending on your thoughts on my feedback below.

I'm looking at the deployed office policy and I now see why you made the css changes. This looks like the result of no margin around ul elements. I think it is reasonable to consider changing their rules instead of the headers'.

If you use an inspection tool you'll see that paragraphs have top and bottom margin, headers have bottom margin, and lists have no margin at all. I think lists should match paragraph spacing. What do you think?

ehashman commented 8 years ago

@leafa: can you rebase this commit on top of master so we can see what the css changes look like?

mxyang42 commented 8 years ago

@ehashman Sure! Sorry, I meant to do this for a while, but my laptop wasn't available until now.

mxyang42 commented 8 years ago

@ehashman You should be able to see the css changes now. I grouped the margin-bottom for everything together - headers, paragraphs and lists alike.

ehashman commented 8 years ago

Cool, I will redeploy this when Heroku stops being broken. (lol... submitted a support ticket)

ehashman commented 8 years ago

The review app was broken, seeing if this fixes it.

ehashman commented 8 years ago

Doesn't seem to have. Let's close this for now until someone can test this against master, it's gotten pretty out of date.