-
Notifications
You must be signed in to change notification settings - Fork 995
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
Conversation
✅ Deploy Preview for redwoodjs-docs canceled.
|
For apps, this setting is code via editorConfig rather than VSCode:
|
And also in the framework in
Is there a way to ignore markdown files using editorConfig rather than VSCode settings? |
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 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? |
@dthyresson, @Philzen had this question for you, can you take a look please?
|
For me, it is because EditorConfig supports multiple editors and doesn't assume devs will use VSCode. See:
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. |
@dthyresson Totally with you on that one.
I misunderstood that changes to |
Question, should these editorConfig's be added to
Not just in the project template? |
Oh @Philzen I just saw that this is in the framework's editorConfig:
So, maybe this configuration is better for 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. |
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 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. |
🌟 |
@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 ReportCI 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. |
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 @jtoar @Tobbe seems nobody is assigned to this anymore – just a little bump so it's not forgotten 😏 @Tobbe wrote:
|
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? |
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 But i strongly think the settings suggested in this PR should be the default and not vice-versa for the following reasons:
(BTW i've used trailing whitespace to format this markdown comment – something that Github respects but the current Redwood settings sabotage 😉 ) |
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? |
I remember: #5868 (comment) (have it on my TODO list) |
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
…ject + create-redwood-app template) (#5869) Co-authored-by: Tobbe Lundberg <tobbe@tlundberg.com>
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:
.editorconfig
in the RedwoodJS main project