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 info about MultilineBracketStyle to docs, fix issue with icons #2744

Merged
merged 7 commits into from
Jan 25, 2023

Conversation

josh-degraw
Copy link
Contributor

@josh-degraw josh-degraw commented Jan 23, 2023

Closes #2743, related to #2710

@josh-degraw josh-degraw self-assigned this Jan 23, 2023
Comment on lines 21 to 26
case 'deprecated':
settingType = {
icon: "bi-exclamation-triangle-fill",
color: "orange-recommendation",
tooltip: "This setting is deprecated and will be removed in a future version."
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Open to discussion if there's a better way to display this, I just figured there should be some sort of callout for deprecated settings.

@nojaf
Copy link
Contributor

nojaf commented Jan 24, 2023

I'm on the fence about this one. Your changes are fine and I can see value in having them.
What I'm struggling with is:

  • Should we even keep the old settings documentation when the new one is working? I removed it from the online tool.
  • Is documentation really going to cover it? Should we not think about printing a warning in the cli as well? The warning goes away once you have updated the .editorconfig.
  • The configuration.fsx page also shows the discrepancy between the .editorconfig and FormatConfig. On a strictly technical level, v5.2 has a breaking change in Fantomas.Core 😔 As the old setting MultilineBlockBracketsOnSameColumn is no longer available there. So maybe it is better to just move forward and don't mention the old stuff anymore as a separate section. But rather as a remark in the new setting.

My availability is limited this week, but I trust you and @dawedawe will figure it out 😉.

@josh-degraw
Copy link
Contributor Author

Yeah, I think it could make sense to just remove the old settings with just a mention of the old settings underneath the new one just to help anyone looking to ctrl+f one of the old ones. I also wouldn't be opposed to a cli warning as well. We'd probably want to do that in a separate PR, no?

@nojaf
Copy link
Contributor

nojaf commented Jan 25, 2023

Skimmed true this, looks good. I'm going to let @dawedawe review it properly.

@nojaf nojaf requested a review from dawedawe January 25, 2023 17:48
Copy link
Member

@dawedawe dawedawe left a comment

Choose a reason for hiding this comment

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

I would also prefer to have a separate PR for the CLI warning.

docs/content/webcomponents.js Outdated Show resolved Hide resolved
docs/content/webcomponents.js Outdated Show resolved Hide resolved
docs/content/webcomponents.js Outdated Show resolved Hide resolved
@josh-degraw josh-degraw enabled auto-merge January 25, 2023 22:21
@josh-degraw josh-degraw disabled auto-merge January 25, 2023 22:22
@josh-degraw josh-degraw enabled auto-merge January 25, 2023 22:22
@josh-degraw josh-degraw merged commit 646d19d into fsprojects:main Jan 25, 2023
@josh-degraw josh-degraw deleted the docs-update branch January 25, 2023 22:30
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.

Missing docs on how to use multiline_bracket_style
3 participants