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

adjusted css for consistency of spacing #115

Closed
wants to merge 2 commits into from

Conversation

mxyang42
Copy link
Contributor

@mxyang42 mxyang42 commented Oct 8, 2015

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 ehashman temporarily deployed to wics-site-pr-115 October 11, 2015 20:42 Inactive
@ehashman
Copy link
Contributor

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.)


# Personnel #

* If the office is open for office hours, drop-ins or for any other reason, a MINIMUM of ONE committee member of WiCS must be present in the office at all times.
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be 80-char justified.

I'm also not a fan of the use of caps in this document. I'd recommend using a bold font instead.

@mxyang42
Copy link
Contributor Author

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
Copy link
Contributor Author

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

@evykassirer
Copy link
Member

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

@ehashman
Copy link
Contributor

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 added a commit that referenced this pull request Oct 26, 2015
@ehashman ehashman changed the title added office policy and adjusted css for consistency of spacing adjusted css for consistency of spacing Oct 30, 2015
@ehashman
Copy link
Contributor

ehashman commented Nov 8, 2015

@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?

## Office Hours ##

* Office hours and services are open only to ciswomen, transwomen and non-binary
folks.
Copy link
Contributor

Choose a reason for hiding this comment

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

Also one last detail: this line should read "cis women, trans women, and non-binary individuals." to be consistent with the rest of our content.

@ehashman
Copy link
Contributor

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

@mxyang42
Copy link
Contributor Author

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

@mxyang42
Copy link
Contributor Author

@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
Copy link
Contributor

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

@ehashman
Copy link
Contributor

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

@ehashman ehashman reopened this Apr 29, 2016
@ehashman
Copy link
Contributor

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.

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