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

Add 2.5 blog #11823

Merged
merged 42 commits into from Nov 25, 2021
Merged

Add 2.5 blog #11823

merged 42 commits into from Nov 25, 2021

Conversation

sosukesuzuki
Copy link
Member

@sosukesuzuki sosukesuzuki commented Nov 17, 2021

@sosukesuzuki sosukesuzuki mentioned this pull request Nov 17, 2021
12 tasks

<!-- Prettier 2.5 -->
```tsx
const test = <T>(value: T) => {};
Copy link
Sponsor Member

Choose a reason for hiding this comment

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

Is this the output in 2.4?

@kachkaev
Copy link
Member

kachkaev commented Nov 18, 2021

If anyone wants to proofread the post, note that Preview URL has changed:

https://deploy-preview-11823--prettier.netlify.app/blog/2021/11/17/2.5.0.html
https://deploy-preview-11823--prettier.netlify.app/blog/2021/11/25/2.5.0.html

@sosukesuzuki sosukesuzuki force-pushed the add-2.5-blog branch 2 times, most recently from 5a31ea7 to b2ae0f4 Compare November 21, 2021 06:55
@sosukesuzuki sosukesuzuki marked this pull request as ready for review November 21, 2021 16:59
Copy link
Member

@j-f1 j-f1 left a comment

Choose a reason for hiding this comment

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

Made a bunch of suggestions. I’m not particularly attached to them, so feel free to reject any you don’t agree with :)


Starting with Prettier 2.3.0, type declarations in arrow functions could affect function body offset. Changing the length of the type annotation could produce large diffs and thus increased the chance of git conflicts. To prevent this, function body offset was stabilized.
However, this change can make a big difference from 2.4.1. Please be careful.
Copy link
Member

Choose a reason for hiding this comment

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

The use of the term “offset” here wasn’t clear to me — maybe change it to “indentation?” Additionally, I would reword the last sentence like so:

Suggested change
However, this change can make a big difference from 2.4.1. Please be careful.
**Note**: This change may affect a large number of lines in your codebase.

(or something similar?)

changelog_unreleased/typescript/11721.md Outdated Show resolved Hide resolved
changelog_unreleased/typescript/11721.md Show resolved Hide resolved
changelog_unreleased/typescript/11721.md Show resolved Hide resolved
changelog_unreleased/mdx/11563.md Outdated Show resolved Hide resolved
website/blog/2021-11-21-2.5.0.md Outdated Show resolved Hide resolved
website/blog/2021-11-21-2.5.0.md Outdated Show resolved Hide resolved
website/blog/2021-11-21-2.5.0.md Outdated Show resolved Hide resolved
changelog_unreleased/html/11827.md Outdated Show resolved Hide resolved
website/blog/2021-11-21-2.5.0.md Outdated Show resolved Hide resolved
changelog_unreleased/BLOG_POST_INTRO_TEMPLATE.md Outdated Show resolved Hide resolved
changelog_unreleased/blog-post-intro.md Outdated Show resolved Hide resolved
changelog_unreleased/typescript/11721.md Outdated Show resolved Hide resolved
Copy link
Member

@j-f1 j-f1 left a comment

Choose a reason for hiding this comment

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

LGTM with one small formatting suggestion:

changelog_unreleased/typescript/11515.md Outdated Show resolved Hide resolved
@j-f1
Copy link
Member

j-f1 commented Nov 24, 2021

I just noticed that #11823 (comment) was marked as resolved but my first sentence about the use of “offset” was not addressed.

@sosukesuzuki
Copy link
Member Author

@j-f1 @kachkaev Thank you for your reviewing! I've fixed.

// Input
@use 'library' with (
$black: #222,
$border-radius: 0.1rem $font-family: 'Helvetica, sans-serif'
Copy link

Choose a reason for hiding this comment

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

Is this supposed to be invalid Sass? If not, there's a missing comma. (below as well + newline)

Copy link
Member Author

Choose a reason for hiding this comment

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

I think it is valid scss

Copy link

Choose a reason for hiding this comment

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

How?

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not familiar with SCSS, but at least there is a below code on the scss docs.

@use 'library' with (
  $black: #222,
  $border-radius: 0.1rem
);

https://sass-lang.com/documentation/at-rules/use#configuration

Copy link

@glen-84 glen-84 Nov 25, 2021

Choose a reason for hiding this comment

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

image

There are two variables on the same line, with no comma between them.

Copy link
Member Author

Choose a reason for hiding this comment

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

Woops sorry I didn't notice that. I'll fix later, thank you!

@sosukesuzuki sosukesuzuki merged commit 04aa850 into prettier:main Nov 25, 2021
@kachkaev
Copy link
Member

Great work @sosukesuzuki 👏👏👏

@sosukesuzuki sosukesuzuki deleted the add-2.5-blog branch November 25, 2021 14:16
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Nov 26, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants