Skip to content
This repository has been archived by the owner on Apr 8, 2023. It is now read-only.

fix for issue #112 #116

Merged
merged 3 commits into from May 4, 2019
Merged

Conversation

t-fritsch
Copy link
Contributor

What is the purpose of this pull request?

This PR is intented to resolve issue #112 ie. preserving cursor position and section folding when using "csscomb.formatOnSave": true,

What changes did you make? (Give an overview)

copy/paste of the code running when using Command Palette "CSSComb: format styles" (onCommand) into the code running when saving file (onSave).
There definitely should be some refactoring to do but since I'm not sure of what I'm really doing, I prefer leaving as is, since it seems to be working. 馃帀

@mrmlnc mrmlnc self-assigned this Sep 5, 2018
@mrmlnc
Copy link
Owner

mrmlnc commented Sep 19, 2018

Hello, @tfritsch-km,

Unfortunately, this solution has a regression. For example, after it changes, we will only use the formatter for an open document when many documents have been modified.

Before

  1. Change the smoke/test.css and test.less file (add whitespace, for example)
  2. Run: F1 -> File: Save all
  3. All files will be saved using the formatter.

After

  1. Change the smoke/test.css and test.less file (add whitespace, for example)
  2. Run: F1 -> File: Save all
  3. All files will be saved, but the formatter will be applied only to the current editor.

@t-fritsch
Copy link
Contributor Author

Thank you @mrmlnc for the review. I didn't see the bug since I never use the "save all" feature, thank you for pointing it ! Any idea of a way to do it ?

@mrmlnc
Copy link
Owner

mrmlnc commented Jan 17, 2019

As I said above, this is an incorrect fix that solves only one problem and generates new ones. I can't merge this change as it breaks an already existing behavior.

But, if it is really important for users, then I can merge this changes under one condition: you need to add an option that disables the new behavior by default.

You can also ask the VS Code team about how to fix this problem correctly.

@t-fritsch
Copy link
Contributor Author

@mrmlnc I just pushed a fix for the case you mentioned (Save all).
If you've got some time to give it a try, I think it works quite well (even i the code itselft, is not really what I call beautiful waiting for microsoft/vscode#15178 to evolve) 馃槃

@mrmlnc mrmlnc self-requested a review February 4, 2019 05:50
@t-fritsch
Copy link
Contributor Author

Hello @mrmlnc have you found any new bug on this PR ? Any chance it gets merged ?

@t-fritsch
Copy link
Contributor Author

Hello @mrmlnc sorry to insist but is there something I could do to help or ease the review process ?
You asked me to do a PR -that I did 3 months ago- but maybe there is something that is missing in what I did ?
Feel free to tell me if you need something more from me, I understand that you've got plenty of work with others repos, but I think this PR could help people working with this extension and csscomb 馃槈

@mrmlnc mrmlnc merged commit a3da0bf into mrmlnc:master May 4, 2019
@mrmlnc
Copy link
Owner

mrmlnc commented May 4, 2019

Thank you for that input, @tfritsch-km 馃帀 馃尞

I am sorry to everyone for the long wait.

I will add just one commit that will correct the formatting and predicate of the condition for finding a similar document.

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

3 participants