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

Fixed #59395 - Emmet Syntax Profiles tag_nl produces no extra space #60108

Merged
merged 3 commits into from Oct 23, 2018

Conversation

Projects
None yet
3 participants
@irrationalRock
Contributor

irrationalRock commented Oct 7, 2018

This PR fixes #59395.

With:

"emmet.syntaxProfiles": {
        "html": {
            "tag_nl": false            
        }
    },

It no longer allows extra space.

Before:
vscode-59395-before

After:
vscode-59395

Thanks for considering this request.

@ramya-rao-a

ramya-rao-a requested changes Oct 8, 2018 edited

The tests seem to be failing. Can you take a look at that?

To run the tests locally, run scripts\test-integration.bat on Windows or ./scripts/test-integration.sh on Linux/Mac

It would also do us good to add a test case to cover this scenario.
wrapWithAbbreviation.test.ts would be the file to update.

// If wrapping with a block element, insert newline in the text to wrap.
if (wrappingNode && inlineElements.indexOf(wrappingNode.name) === -1) {
if (wrappingNode && inlineElements.indexOf(wrappingNode.name) === -1 && emmetConfig.syntaxProfiles['html']['tag_nl']) {

This comment has been minimized.

@ramya-rao-a

ramya-rao-a Oct 8, 2018

Member

We can probably get the value for tag_nl from expandOptions.profile object

@irrationalRock

This comment has been minimized.

Contributor

irrationalRock commented Oct 10, 2018

Thanks for the feedback. I'm looking at the tests and will update tag_nl to expandOptions.profile

@irrationalRock

This comment has been minimized.

Contributor

irrationalRock commented Oct 11, 2018

I updated my code to use expandOptions.profile. I'm wondering if you can give me an example of how to change the config in tests. I am having some problems trying to change tag_nl to false.

@ramya-rao-a

My apologies, I should have cleared some things previously.

The new emmet modules that we use dont use the same syntaxProfile settings as documented in https://docs.emmet.io/customization/syntax-profiles/

The new emmet modules follow the profile as defined in https://github.com/emmetio/output-profile/blob/1a7571e78da9d7bf056289b3396dbb0bcc45c435/types.d.ts#L1

To support backward compatibility in VS Code, we support both in the settings. I convert the old profile to a new one using a helper

So now coming to the current issue. You can just add expandOptions['format'] === true to the if condition and that should be enough.

@ramya-rao-a

This comment has been minimized.

Member

ramya-rao-a commented Oct 12, 2018

I'm wondering if you can give me an example of how to change the config in tests.

https://github.com/Microsoft/vscode/blob/release/1.28/extensions/emmet/src/test/abbreviationAction.test.ts#L441 is an example where I changed the config for the emmet.excludeLanguages setting. You can use that as an example.

@irrationalRock

This comment has been minimized.

Contributor

irrationalRock commented Oct 17, 2018

Thanks for the help. I had to use expandOptions['profile'].format instead of expandOptions['format'] because it didn't seem to produce any properties when tag_nl was set to true.
I also added tests for this feature.

irrationalRock added some commits Oct 20, 2018

@irrationalRock irrationalRock force-pushed the irrationalRock:fix-59395 branch from 20f753a to af700bd Oct 20, 2018

@irrationalRock

This comment has been minimized.

Contributor

irrationalRock commented Oct 20, 2018

It seems like my commits fail when running the hygiene tests. Is there any way for me to run these tests on my local machine? Running ./scripts/test-integration.sh passes all of the tests.

@ramya-rao-a

This comment has been minimized.

Member

ramya-rao-a commented Oct 23, 2018

The hygiene failure was due to formatting errors. I have pushed a change to fix that.

@ramya-rao-a ramya-rao-a merged commit 19ac31a into Microsoft:master Oct 23, 2018

2 checks passed

VS Code #20181023.3 succeeded
Details
license/cla All CLA requirements met.
Details

@ramya-rao-a ramya-rao-a added this to the October 2018 milestone Oct 23, 2018

@contentfree

This comment has been minimized.

contentfree commented Nov 8, 2018

Wrap still produces incorrect results if tag_nl_leaf is false, I believe. (tag_nl_leaf doesn't seem to be respected at all, really, nor does the 'decide' value for tag_nl)

@ramya-rao-a

This comment has been minimized.

Member

ramya-rao-a commented Nov 8, 2018

@contentfree I have logged an upstream issue for that. See emmetio/output-profile#1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment