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

Preserve trailing whitespace for Markdown, HTML & MJML (RedwoodJS project + create-redwood-app template) #5869

Merged
merged 5 commits into from
Dec 18, 2023

Conversation

Philzen
Copy link
Contributor

@Philzen Philzen commented Jul 3, 2022

While trailing whitespace is just bit noise in code (well, 99 % of the time...), for markdown it has actual signifiance.

This used to drive me crazy for a while until i found this setting. This will save others the trouble.


There was some scope creep in the discussion below, so this PR now both introduces changes and also fixes intended behaviour, namely:

  • (Fix) Bring VSCode settings in line with .editorconfig in the RedwoodJS main project
    • this is particularly nice b/c trailing whitespace is now preserved when editing the docs
  • (change) Not only preserve whitespace for markdown, but also HTML & MJML files in the project template

@netlify
Copy link

netlify bot commented Jul 3, 2022

Deploy Preview for redwoodjs-docs canceled.

Name Link
🔨 Latest commit 33bec16
🔍 Latest deploy log https://app.netlify.com/sites/redwoodjs-docs/deploys/62f100411a37180008f39613

@Philzen Philzen changed the title Leave markdown trailing whitespace untouched Preserve trailing whitespace when editing markdown files Jul 3, 2022
@dthyresson
Copy link
Contributor

For apps, this setting is code via editorConfig rather than VSCode:

trim_trailing_whitespace = true

@dthyresson
Copy link
Contributor

dthyresson commented Jul 3, 2022

And also in the framework in

trim_trailing_whitespace = true

Is there a way to ignore markdown files using editorConfig rather than VSCode settings?

@Philzen
Copy link
Contributor Author

Philzen commented Jul 4, 2022

For apps, this setting is code via editorConfig rather than VSCode:

trim_trailing_whitespace = true

Thanks @dthyresson good catch! I just checked in a test project ... it's crazy but they are both required (→ fix in 1f37abb) – it seems otherwise any true value takes precedence over the other.

I urge you to test out yourself, but from my tests it seems there's no way around adding the exclusion to both VSCode and .editorconfig to make it stop removing our precious markdown line-breaks on save.

May i ask if there's any reason changing the .vscode/settings.json should be discouraged?

@Tobbe Tobbe added release:feature This PR introduces a new feature labels Jul 5, 2022
@Tobbe
Copy link
Member

Tobbe commented Jul 5, 2022

@dthyresson, @Philzen had this question for you, can you take a look please?

May i ask if there's any reason changing the .vscode/settings.json should be discouraged?

@dthyresson
Copy link
Contributor

dthyresson commented Jul 5, 2022

May i ask if there's any reason changing the .vscode/settings.json should be discouraged?

For me, it is because EditorConfig supports multiple editors and doesn't assume devs will use VSCode.

See:

These editors come bundled with native support for EditorConfig. Everything should just work.

image

Therefore, it is a more general solution.

For example, if I edit the markdown file within GitHub, it will respect the rules set in the configuration.

@Philzen
Copy link
Contributor Author

Philzen commented Jul 5, 2022

@dthyresson Totally with you on that one.

Is there a way to ignore markdown files using editorConfig rather than VSCode settings?

I misunderstood that changes to .vscode/settings.json were discouraged in general, which is not the case (and actually required for this one). We need to treat .editorconfig as the first-class cititzen, absolutely.

@dthyresson
Copy link
Contributor

Question, should these editorConfig's be added to

  • framework root editorConfig (ie, handle markdown files in framework)
  • the /docs editorConfig for Docusaurus since these are all markdown file, too
    ?

Not just in the project template?

@dthyresson
Copy link
Contributor

dthyresson commented Jul 5, 2022

Oh @Philzen I just saw that this is in the framework's editorConfig:

[*.{md,html,mjml}]
trim_trailing_whitespace = false

So, maybe this configuration is better for the project template?

@Philzen
Copy link
Contributor Author

Philzen commented Jul 10, 2022

Question, should these editorConfig's be added to

  • framework root editorConfig (ie, handle markdown files in framework)
  • the /docs editorConfig for Docusaurus since these are all markdown file, too
    ?

Not just in the project template?

Excellent catch! This adds even more value for docs contributors that work on their fork locally. Updated in 1f35f48 – now also works like a charm in the docs folder.

@Philzen
Copy link
Contributor Author

Philzen commented Jul 10, 2022

[*.{md,html,mjml}]
trim_trailing_whitespace = false

So, maybe this configuration is better for the project template?

I sense intensive scope creep here 🧐 😉 … but so be it, as i understand there seems to be a consensus amongst the core team and also widely across the rest of the community that you want full control over your own whitespace in HTML & MJML as well. I can actually relate to that, i had cases where i wished it would respect my plain content within JSX tags and had to work around the editor's interference by either putting   or prerendering a variable that includes my fully intended whitespace.

Note that this setting was actually never respected by VSCode before … until now: dcd9ba0

bec87ab introduces the same setting consistently for the project template as well.


I've updated the issue title & description accordingly so the whole scope of this PR is transparent in the commit history once merged.

@Philzen Philzen changed the title Preserve trailing whitespace when editing markdown files Preserve trailing whitespace for Markdown, HTML & MJML (RedwoodJS project + create-redwood-app) Jul 10, 2022
@Philzen Philzen changed the title Preserve trailing whitespace for Markdown, HTML & MJML (RedwoodJS project + create-redwood-app) Preserve trailing whitespace for Markdown, HTML & MJML (RedwoodJS project + create-redwood-app template) Jul 10, 2022
@Tobbe
Copy link
Member

Tobbe commented Jul 10, 2022

I've updated the issue title & description accordingly so the whole scope of this PR is transparent in the commit history once merged.

🌟

@jtoar
Copy link
Contributor

jtoar commented Jul 11, 2022

@dthyresson thanks for moving this one along! As this was opened before the bot started assigning people to PRs, I just 1) assigned you to it and 2) put it into the current cycle cause it feels like it's out of triage, but feel free to undo if that's not what you had in mind!

@nx-cloud
Copy link

nx-cloud bot commented Jul 22, 2022

☁️ Nx Cloud Report

CI is running/has finished running commands for commit 33bec16. As they complete they will appear below. Click to see the status, the terminal output, and the build insights.

📂 See all runs for this branch


✅ Successfully ran 14 targets

Sent with 💌 from NxCloud.

@Tobbe
Copy link
Member

Tobbe commented Jul 22, 2022

To me it sounds like all your comments were addressed @dthyresson. Anything preventing us from merging this? (Except for failing tests, but those seems unrelated - I've triggered a new round of tests now)

@Philzen
Copy link
Contributor Author

Philzen commented Jul 28, 2022

@dthyresson @jtoar @Tobbe seems nobody is assigned to this anymore – just a little bump so it's not forgotten 😏

@Tobbe wrote:

To me it sounds like all your comments were addressed @dthyresson. Anything preventing us from merging this? (Except for failing tests, but those seems unrelated - I've triggered a new round of tests now)

@dthyresson
Copy link
Contributor

Having looked at the changes, I realized this change needs documentation to explain why developers would see there behavior of whitespace not being trimmed in these cases.

But, if we have to document why this is happening and how to revert the behavior, I propose instead to simply add documentation to say "How to configure trailing whitespace in your RedwoodJS project".

And then developers can add these settings themselves.

Thoughts?

@Philzen
Copy link
Contributor Author

Philzen commented Aug 6, 2022

I do support the notion of adding some info regarding this (i guess https://redwoodjs.com/docs/project-configuration-dev-test-build would be a good place) – after all, working with Redwood and its docs taught me many new things, so showing how .editorconfig and ESLint/prettier overshadow each other and how to make them behave sounds like a worthwhile addition 👍

But i strongly think the settings suggested in this PR should be the default and not vice-versa for the following reasons:

  • As stated in the initial description, the current setting interrupted my work and thus sidetracked me for a consiberable amount of time until i was able to find a fix, time in which i could not make progress on actual implementations. As Redwood is an opinionated project, if feel it should strive for the best possible DX and avoid people bumping into such an issue being caused by its default settings

  • whitespaces in Markdown do have significance, so imho the current setting makes files invalid on save

  • basically this PR fixes an existing inconsistency b/c the .editorconfig in the root folder already has that setting (for good reasons) – it is only overruled b/c ESLint/VSCode settings are not harmonized with it:

    redwood/.editorconfig

    Lines 17 to 18 in 2513813

    [*.{md,html,mjml}]
    trim_trailing_whitespace = false

(BTW i've used trailing whitespace to format this markdown comment – something that Github respects but the current Redwood settings sabotage 😉 )

@Tobbe
Copy link
Member

Tobbe commented Aug 8, 2022

I do support the notion of adding some info regarding this (i guess redwoodjs.com/docs/project-configuration-dev-test-build would be a good place)

Yes, that would be the place. We actually discussed adding more info there about how to configure your RW App DX settings. Like if you wanted to further configure (or turn off) import order formatting. And I think there was one more thing, but I can't remember right now. @jtoar Do you remember?

@Philzen
Copy link
Contributor Author

Philzen commented Aug 8, 2022

I remember: #5868 (comment) (have it on my TODO list)

Philzen and others added 4 commits December 18, 2023 13:32
Setting are inherited throughout the whole project, so this
also fixes whitespace editing for the docs.
… project)

Basically a fix to bring VSCode bahaviour in line with what's
already defined in ./.editorconfig
@Tobbe Tobbe enabled auto-merge (squash) December 18, 2023 13:00
@Tobbe Tobbe merged commit 5254382 into redwoodjs:main Dec 18, 2023
32 checks passed
Tobbe added a commit that referenced this pull request Dec 21, 2023
…ject + create-redwood-app template) (#5869)

Co-authored-by: Tobbe Lundberg <tobbe@tlundberg.com>
@Tobbe Tobbe modified the milestones: next-release, v6.6.0 Dec 22, 2023
@Philzen Philzen deleted the patch-7 branch April 24, 2024 13:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release:feature This PR introduces a new feature
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

4 participants