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

Blankspace taming #341

Merged
merged 2 commits into from
Apr 24, 2019
Merged

Blankspace taming #341

merged 2 commits into from
Apr 24, 2019

Conversation

craftninja
Copy link
Contributor

Corrected some markdown table issues and a typo. No data change!

blocks stretching back and forth

@craftninja
Copy link
Contributor Author

craftninja commented Apr 15, 2019

I'd be glad to add the olde PR links to already merged peeps' data rows if that is helpful, either in this or another (new) PR.

UPDATE: looks like @sagirk is working on that in #342

@Trott
Copy link
Member

Trott commented Apr 15, 2019

Not opposed to this, but for the record, blank spaces were added to make the table readable as raw text (at least with wrapping disabled and, of course, with a monospace typeface), although obviously that tends to deteriorate over time. I'd slightly prefer the spaces to be added back in a way that aligns the table again, but this is fine too.

Copy link
Member

@Trott Trott left a comment

Choose a reason for hiding this comment

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

LGTM despite my (mild) preference for the spaces/aligning stuff. Needs merge conflicts resolved, and that's probably going to happen a lot with this one, so I wouldn't mind it if we make a point to land it soon. It's not like spaces aren't easy to add back in if anyone finds them crucial somehow.

@Trott
Copy link
Member

Trott commented Apr 15, 2019

(This also points to the probability that a markdown table might not be the best format, but I'm not sure I have a better one to suggest, at least not within the context of our pull request-based workflow.)

@craftninja
Copy link
Contributor Author

@Trott I was referencing the previous years for guidance, and also mild preference for less blankspace... with the thought that maybe this makes it easier for people to keep columns aligned? I'll keep addressing merge conflicts / column misalignments in the interim ✨

@SofiEstevez
Copy link
Contributor

@mhdawson @Bamieh tagging you to approve and merge this :)

@mhdawson
Copy link
Member

I think it needs a rebase after the previous items that have landed

@craftninja
Copy link
Contributor Author

@mhdawson Updated!

@Trott Trott merged commit a1430b3 into nodejs:master Apr 24, 2019
@craftninja craftninja deleted the blankspace-taming branch April 24, 2019 16:05
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.

4 participants