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

header.hbs: Fix wrong logo width #2623

Merged
merged 2 commits into from
Oct 9, 2019
Merged

header.hbs: Fix wrong logo width #2623

merged 2 commits into from
Oct 9, 2019

Conversation

XhmikosR
Copy link
Contributor

@XhmikosR XhmikosR commented Sep 29, 2019

No description provided.

@XhmikosR XhmikosR changed the title Update header.hbs header.hbs: Fix wrong logo width Sep 29, 2019
@XhmikosR
Copy link
Contributor Author

XhmikosR commented Oct 3, 2019

/CC @Trott

@XhmikosR
Copy link
Contributor Author

XhmikosR commented Oct 8, 2019

Can we get this merged?

@Trott
Copy link
Member

Trott commented Oct 9, 2019

@nodejs/website

@phillipj
Copy link
Member

phillipj commented Oct 9, 2019

@XhmikosR mind elaborating a bit? Wrong, how/why?

@XhmikosR
Copy link
Contributor Author

XhmikosR commented Oct 9, 2019

The SVG has these:

width="589.827" height="361.238" viewBox="0 0 442.37 270.929"

So a ratio of 1.6327. 180/75 = 2.4.

If you notice the current image, it has blanks to the right and left; it's due to this.

So, this just makes sure we use the right width.

@phillipj
Copy link
Member

phillipj commented Oct 9, 2019

Great explanation, much appreciated 👏

Copy link
Member

@phillipj phillipj left a comment

Choose a reason for hiding this comment

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

LGTM

@phillipj
Copy link
Member

phillipj commented Oct 9, 2019

@XhmikosR you want to get in sync with master by rebasing and force push? Based on my previous experience with your PRs, I'm assuming you want as clean git commit history as possible. If not, give me a wink and I'll use GitHub's UI to merge the latest changes from master into this branch.

@XhmikosR
Copy link
Contributor Author

XhmikosR commented Oct 9, 2019

@phillipj: now the option for branches to be up to date is enabled for the repo, so this is no longer an issue :)

When I have one commit I usually prefer squash and merge so that the PR number is included in the commit.

@XhmikosR XhmikosR merged commit 60bd43c into nodejs:master Oct 9, 2019
@XhmikosR XhmikosR deleted the patch-2 branch October 9, 2019 09:12
@phillipj
Copy link
Member

phillipj commented Oct 9, 2019

now the option for branches to be up to date is enabled for the repo, so this is no longer an issue

And as clicking that button on the GitHub UI which "updates" the branch, will cause a merge-commit from master into this branch, there will be more commits than you've created yourself. Since you've been very explicit multiple times about collaborators not squashing your commits before merging, I asked you beforehand.

Also would much rather prefer if we came to a consensus about squashing, rebasing etc and documented in COLLABORATOR_GUIDE.md, similar to what is found in nodejs/node/COLLABORATOR_GUIDE.md.

@XhmikosR
Copy link
Contributor Author

XhmikosR commented Oct 9, 2019

@phillipj that's correct, but if you choose "Rebase and Merge" there will be no merge commits.

Personally, I have the Merge option disabled in the repo settings.

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.

None yet

4 participants