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

Hint compat api html #1584

Merged
merged 49 commits into from Jan 7, 2019

Conversation

Projects
None yet
6 participants
@borgitas21
Copy link
Contributor

borgitas21 commented Dec 14, 2018

This PR implements the HTML hints. More info webhintio/rfcs#10

To do for HTML part of this task:

  • Implement walk feature.
  • Use Location into reports
  • Don't check more than once the same scenario (cache)

Feature: Which properties or values are deprecated in the HTML

  • Check element tags.
  • Check element attributes.
  • Check global attributes.
  • Tests
  • Docs

Feature: Which properties or values are not broadly supported in the HTML.

  • Check element tags.
  • Check element attributes.
  • Check global attributes.
  • Tests
  • Docs

@borgitas21 borgitas21 requested review from alrra , antross , molant and sarvaje as code owners Dec 14, 2018

@borgitas21 borgitas21 changed the title git cFeature/hint compat api html Hint compat api html Dec 14, 2018

Show resolved Hide resolved packages/hint-compat-api/src/meta/html-next.ts Outdated
Show resolved Hide resolved packages/hint-compat-api/src/meta/html.ts Outdated
Show resolved Hide resolved packages/hint-compat-api/src/core/api-hint.ts Outdated
Show resolved Hide resolved packages/hint-compat-api/src/core/api-hint.ts Outdated
Show resolved Hide resolved packages/hint-compat-api/src/core/api-hint.ts Outdated
Show resolved Hide resolved packages/hint-compat-api/src/core/api-hint.ts Outdated
Show resolved Hide resolved packages/hint-compat-api/src/enums.ts Outdated
@antross
Copy link
Member

antross left a comment

I ran into one other issue after spinning this up in the VS Code extension. I'm getting correct reports on the first pass over a file, but after making an edit all of the reports from the compat api hints (both CSS and HTML) disappear. Perhaps some sort of caching issue?

If you want to try this yourself:

  • Edit packages/configuration-development/index.json to include:
        "compat-api/css": "error",
        "compat-api/css-next": "error",
        "compat-api/html": "error",
        "compat-api/html-next": "error",
  • Run yarn build from the root of the repo
  • Go to "File > Open Folder" in VS Code and choose packages/extension-vscode
  • Go to the "Debug" panel on the left
  • Choose "Client + Server" from the "DEBUG" drop down
  • Click the "Start Debugging" icon

This will open a new VS Code window. In that window:

  • Go to "File > Open Folder" and choose the root folder of the repo
  • Open one of the files in packages/hint-compat-api/tests/fixtures

At this point webhint should run automatically and start showing errors in VS Code. Once you see an initial error, make any edit to the file and the ones from hint-compat-api will disappear (but other webhint errors will still remain).

@borgitas21 borgitas21 force-pushed the CKGrafico:feature/hint-compat-html branch from 5f2b7bb to 634544f Dec 18, 2018

@borgitas21 borgitas21 force-pushed the CKGrafico:feature/hint-compat-html branch from 634544f to a7a4119 Dec 18, 2018

@borgitas21

This comment was marked as resolved.

Copy link
Contributor

borgitas21 commented Dec 18, 2018

@antross after following the steps you mentioned above, I was not able to reproduce the error.
I'm opening a fixture file but nothing is hapenning, any idea?

At this point webhint should run automatically and start showing errors in VS Code. Once you see an initial error, make any edit to the file and the ones from hint-compat-api will disappear (but other webhint errors will still remain).

Show resolved Hide resolved packages/hint-compat-api/package.json Outdated
Show resolved Hide resolved packages/hint-compat-api/package.json Outdated
@antross
Copy link
Member

antross left a comment

Looks like the deprecated distinction is partly working, but a few non-deprecated CSS properties are still getting reported by both the css and css-next hints (e.g. cursor, resize, text-decoration-color).

I double-checked the MDN data for cursor and resize and confirmed they are both marked "deprecated": false as expected.

.example {
    cursor: pointer;
    resize: both;
}

Actual:

[webhint] cursor is not supported by webview_android. (compat-api/css)
[webhint] cursor is not supported by webview_android. (compat-api/css-next)
[webhint] resize is not supported by edge, ie. (compat-api/css)
[webhint] resize is not supported by edge, ie. (compat-api/css-next)

Expected:

[webhint] cursor is not supported by webview_android. (compat-api/css-next)
[webhint] resize is not supported by edge, ie. (compat-api/css-next)

@alrra alrra force-pushed the webhintio:master branch from 6a9ac06 to 9b2a76e Jan 2, 2019

Show resolved Hide resolved packages/hint-compat-api/docs/compat-api-html-next.md Outdated
Show resolved Hide resolved packages/hint-compat-api/docs/compat-api-html-next.md Outdated
Show resolved Hide resolved packages/hint-compat-api/docs/compat-api-html-next.md Outdated
Show resolved Hide resolved packages/hint-compat-api/docs/compat-api-html-next.md Outdated
Show resolved Hide resolved packages/hint-compat-api/docs/compat-api-html-next.md Outdated
Show resolved Hide resolved packages/hint-compat-api/docs/compat-api-html-next.md Outdated
Show resolved Hide resolved packages/hint-compat-api/docs/compat-api-html.md Outdated
Show resolved Hide resolved packages/hint-compat-api/docs/compat-api-html.md Outdated
Show resolved Hide resolved packages/hint-compat-api/docs/compat-api-html.md Outdated
Show resolved Hide resolved packages/hint-compat-api/docs/compat-api-html.md Outdated

@borgitas21 borgitas21 force-pushed the CKGrafico:feature/hint-compat-html branch from 8f0f05c to cd71f91 Jan 4, 2019

@borgitas21

This comment has been minimized.

Copy link
Contributor

borgitas21 commented Jan 4, 2019

Looks like the deprecated distinction is partly working, but a few non-deprecated CSS properties are still getting reported by both the css and css-next hints (e.g. cursor, resize, text-decoration-color).

@antross Good catch! I forgot to check the deprecated flag when testing the feature and not just when filtering testable features.

@antross

antross approved these changes Jan 4, 2019

Copy link
Member

antross left a comment

@borgitas21 This looks ready to go for me once @alrra and @sarvaje are happy with it. Thanks for all your work and continued iteration! 🎉

With this done, I'm getting increasingly excited to turn hint-compat-api on-by-default. In case you missed it, I've opened #1596 to make all the configuration changes when we're ready. It includes links to the remaining issues I think we need to address before that happens, but we're getting close!

@borgitas21

This comment has been minimized.

Copy link
Contributor

borgitas21 commented Jan 4, 2019

Great. Thanks, @antross! Hopefully #1598 will be ready soon 😃

@borgitas21 borgitas21 force-pushed the CKGrafico:feature/hint-compat-html branch 2 times, most recently from 4ccad96 to 49dbb71 Jan 7, 2019

@borgitas21 borgitas21 force-pushed the CKGrafico:feature/hint-compat-html branch from 49dbb71 to 50879c1 Jan 7, 2019

@antross

This comment has been minimized.

Copy link
Member

antross commented Jan 7, 2019

@sarvaje Any other comments now that this PR has been updated?

@sarvaje

sarvaje approved these changes Jan 7, 2019

@sarvaje

This comment has been minimized.

Copy link
Member

sarvaje commented Jan 7, 2019

@sarvaje Any other comments now that this PR has been updated?

Nop. LGTM

Chore: Drop unnecessary `dependencies` section
Already covered by `devDependencies` and `peerDependencies`.

@antross antross merged commit ac8e2c4 into webhintio:master Jan 7, 2019

1 of 2 checks passed

continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
licence/cla Contributor License Agreement is signed.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment