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

Fix HTML snippets #175

Merged
merged 5 commits into from Oct 20, 2017

Conversation

Projects
None yet
5 participants
@50Wliu
Member

50Wliu commented Oct 16, 2017

I was using TextMate's selector syntax instead of Atom's CSS syntax by accident. I believe this restores pre-v0.48.0 snippet behavior.

Fixes atom/atom#15819

Fix HTML snippets
Also: prevent them from showing in embedded scripts
@Ben3eeE

This comment has been minimized.

Show comment
Hide comment
@Ben3eeE
Member

Ben3eeE commented Oct 16, 2017

@Ben3eeE

This comment has been minimized.

Show comment
Hide comment
@Ben3eeE

Ben3eeE Oct 16, 2017

Member

Tested this in .html, .hbs and .php files and I can use the snippets again in all of these files.

Member

Ben3eeE commented Oct 16, 2017

Tested this in .html, .hbs and .php files and I can use the snippets again in all of these files.

@as-cii

This comment has been minimized.

Show comment
Hide comment
@as-cii

as-cii Oct 16, 2017

Member

Thanks for jumping on this, @50Wliu! ⚡️

Do you think we could add a test that verifies this behavior doesn't regress anymore in the future? Maybe we could experiment with something as easy as getting the available snippets via the snippets package and checking the length of the returned array?

Member

as-cii commented Oct 16, 2017

Thanks for jumping on this, @50Wliu! ⚡️

Do you think we could add a test that verifies this behavior doesn't regress anymore in the future? Maybe we could experiment with something as easy as getting the available snippets via the snippets package and checking the length of the returned array?

@50Wliu

This comment has been minimized.

Show comment
Hide comment
@50Wliu

50Wliu Oct 16, 2017

Member

I'm 👍 on adding a quick test to assert that there's at least a few snippets returned.

Member

50Wliu commented Oct 16, 2017

I'm 👍 on adding a quick test to assert that there's at least a few snippets returned.

@50Wliu

This comment has been minimized.

Show comment
Hide comment
@50Wliu

50Wliu Oct 17, 2017

Member

It would have been nice to do .text.html - .embedded instead of having to null out every snippet, but it looks like that's blocked on atom/atom#1798 as .text.html:not(.embedded) does not work.

Specs should be passing now though, albeit with some hacky workarounds to get the snippets package to load properly.

Member

50Wliu commented Oct 17, 2017

It would have been nice to do .text.html - .embedded instead of having to null out every snippet, but it looks like that's blocked on atom/atom#1798 as .text.html:not(.embedded) does not work.

Specs should be passing now though, albeit with some hacky workarounds to get the snippets package to load properly.

@iolsen

This comment has been minimized.

Show comment
Hide comment
@iolsen

iolsen Oct 18, 2017

Let's cherry-pick this to 1.22-releases for the next hotfix once it's merged.

iolsen commented Oct 18, 2017

Let's cherry-pick this to 1.22-releases for the next hotfix once it's merged.

@iolsen

This comment has been minimized.

Show comment
Hide comment
@iolsen

iolsen Oct 20, 2017

@jasonrudolph can you give this a quick look? You had some opinions as to how this should be fixed.

Hoping to hotfix this and few other things next week.

iolsen commented Oct 20, 2017

@jasonrudolph can you give this a quick look? You had some opinions as to how this should be fixed.

Hoping to hotfix this and few other things next week.

@jasonrudolph

It would have been nice to do .text.html - .embedded instead of having to null out every snippet, but it looks like that's blocked on atom/atom#1798 as .text.html:not(.embedded) does not work.

I agree that it would be nice to not have to explicitly null out every snippet. Given more time, I'd want to look for another option, but this feels like the expedient choice for fixing this bug prior the upcoming release.

@jasonrudolph can you give this a quick look? You had some opinions as to how this should be fixed.

Sure thing. I was hoping that we could do two things:

  1. Add a smoke test that does a sanity check on the number of available snippets. Such a test would have likely caught the regression reported in atom/atom#15819 before the regression was merged to master. This pull request introduces a smoke test for this exact purpose. 👌
  2. Describe the test plan used to verify the results of this change. @50Wliu: Would you mind summarizing the manual verification steps that you've performed for this change?
beforeEach ->
# FIXME: This should just be atom.packages.loadPackage('snippets'),
# but a bug in PackageManager::resolvePackagePath where it finds language-html's
# `snippets` directory before the actual package necessitates passing an absolute path

This comment has been minimized.

@jasonrudolph

jasonrudolph Oct 20, 2017

Member

How would you feel about including a link to the issue here? That way, future devs will have an easy way to find out whether the issue has been resolved, and that will let them know whether they can change the code to use atom.packages.loadPackage('snippets').

@jasonrudolph

jasonrudolph Oct 20, 2017

Member

How would you feel about including a link to the issue here? That way, future devs will have an easy way to find out whether the issue has been resolved, and that will let them know whether they can change the code to use atom.packages.loadPackage('snippets').

This comment has been minimized.

@50Wliu

50Wliu Oct 20, 2017

Member

That reminds me - I still need to file an issue for it.

@50Wliu

50Wliu Oct 20, 2017

Member

That reminds me - I still need to file an issue for it.

This comment has been minimized.

@50Wliu

50Wliu Oct 20, 2017

Member

Done! See the line below.

@50Wliu

50Wliu Oct 20, 2017

Member

Done! See the line below.

@50Wliu

This comment has been minimized.

Show comment
Hide comment
@50Wliu

50Wliu Oct 20, 2017

Member

Test plan:

  1. Open file and set it to HTML syntax
  2. snippets:available and verify that HTML snippets show up
  3. Type spa and confirm that pressing snippets:expand triggers the <span> snippet.
  4. Trigger the script snippet.
  5. Inside the script: snippets:available and verify that no HTML snippets show up

Repeat for PHP, and additionally ensure that no HTML snippets appear inside PHP tags.

Member

50Wliu commented Oct 20, 2017

Test plan:

  1. Open file and set it to HTML syntax
  2. snippets:available and verify that HTML snippets show up
  3. Type spa and confirm that pressing snippets:expand triggers the <span> snippet.
  4. Trigger the script snippet.
  5. Inside the script: snippets:available and verify that no HTML snippets show up

Repeat for PHP, and additionally ensure that no HTML snippets appear inside PHP tags.

@jasonrudolph

This comment has been minimized.

Show comment
Hide comment
@jasonrudolph

jasonrudolph Oct 20, 2017

Member

@50Wliu: Thanks for sharing that test plan. That looks good. 👍

Member

jasonrudolph commented Oct 20, 2017

@50Wliu: Thanks for sharing that test plan. That looks good. 👍

@jasonrudolph

If everything goes well with the test plan, I'm 👍 on shipping this change.

@50Wliu 50Wliu merged commit 06fb18b into master Oct 20, 2017

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@50Wliu 50Wliu deleted the wl-fix-snippets branch Oct 20, 2017

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