Skip to content
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

New: Checks if any SVG element is created using createElement() #1924

Closed

Conversation

utsavized
Copy link
Contributor


Fix #1625

Pull request checklist

Make sure you:

For non-trivial changes, please make sure you also:

  • Added/Updated related documentation.
  • Added/Updated related tests.

--------------------------------------------------------------

Fix webhintio#1625
@utsavized utsavized added this to the 1902-2 milestone Feb 16, 2019
@utsavized utsavized self-assigned this Feb 16, 2019
packages/hint-create-element-svg/package.json Outdated Show resolved Hide resolved
packages/hint-create-element-svg/package.json Outdated Show resolved Hide resolved
packages/hint-create-element-svg/src/svgElements.ts Outdated Show resolved Hide resolved
Copy link
Member

@antross antross left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So cool. 😎

No rush to address the feedback - I just wanted to get an initial review in while I had the time.

Enjoy your weekend!

packages/hint-create-element-svg/README.md Outdated Show resolved Hide resolved
packages/hint-create-element-svg/src/meta.ts Outdated Show resolved Hide resolved
packages/hint-create-element-svg/tests/tests.ts Outdated Show resolved Hide resolved
packages/hint-create-element-svg/src/hint.ts Outdated Show resolved Hide resolved
@utsavized
Copy link
Contributor Author

@antross @molant @sarvaje using eslint's result location did not work, as it sets line number to the beginning of the script tag and not the full source code. To solve that, I subscribed to the element::script event, and tried to use the script element's location to find the real problem location. However, I had some issues with element.getLocation() being null (I think because it is a promise and isn't resolved until the after the report call is made). So, used the findProblemLocation() function from location-helper to get position of the script element, then offset eslint's result location to be relative to the full source code. Not sure if this is the most optimal solution, but I am getting accurate line/col numbers now.

hint-code

@molant
Copy link
Member

molant commented Feb 20, 2019

I believe the issue with getLocation should be solved once #1943 gets merged (and if not we should make sure it is).

@sarvaje
Copy link
Contributor

sarvaje commented Feb 20, 2019

I believe the issue with getLocation should be solved once #1943 gets merged (and if not we should make sure it is).

Yep, that PR should solve the problem.

@molant
Copy link
Member

molant commented Feb 20, 2019

that PR should solve the problem.

Then maybe we should try to get that one merged asap and update once it's done.

@antross
Copy link
Member

antross commented Feb 20, 2019

Looking at the screenshot suggests the position is still off by a little bit as document is getting underlined instead of createElement...

@utsavized
Copy link
Contributor Author

@antross Fixed.

Copy link
Member

@antross antross left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Functionally this looks great. 👍

Most of my comments are just opportunities to streamline the code a bit or improve wording in the documentation.

I also noticed one potential bug with external script references.

packages/hint-create-element-svg/src/svgElements.ts Outdated Show resolved Hide resolved
packages/hint-create-element-svg/src/hint.ts Outdated Show resolved Hide resolved
packages/hint-create-element-svg/src/hint.ts Outdated Show resolved Hide resolved
packages/hint-create-element-svg/src/hint.ts Outdated Show resolved Hide resolved
packages/hint-create-element-svg/src/hint.ts Outdated Show resolved Hide resolved
packages/hint-create-element-svg/src/hint.ts Outdated Show resolved Hide resolved
packages/hint-create-element-svg/README.md Outdated Show resolved Hide resolved
packages/hint-create-element-svg/README.md Outdated Show resolved Hide resolved
packages/hint-create-element-svg/README.md Outdated Show resolved Hide resolved
@utsavized
Copy link
Contributor Author

@antross Does #1953 mean that I don't have the subscribe to element::script anymore, and have access to the element directly from ScriptParse object?

@utsavized
Copy link
Contributor Author

@antross @molant @sarvaje PR Feedback addressed. Thanks!

@utsavized utsavized modified the milestones: 1902-2, 1903-1 Feb 25, 2019
packages/hint-create-element-svg/src/hint.ts Outdated Show resolved Hide resolved
packages/hint-create-element-svg/src/hint.ts Outdated Show resolved Hide resolved
packages/hint-create-element-svg/src/hint.ts Outdated Show resolved Hide resolved
@antross antross modified the milestones: 1903-1, 1903-2 Mar 8, 2019
@utsavized utsavized requested a review from antross March 19, 2019 17:48
packages/hint-create-element-svg/README.md Outdated Show resolved Hide resolved
packages/hint-create-element-svg/src/hint.ts Outdated Show resolved Hide resolved
packages/hint-create-element-svg/src/hint.ts Outdated Show resolved Hide resolved
packages/hint-create-element-svg/src/hint.ts Outdated Show resolved Hide resolved
@utsavized utsavized requested a review from antross March 19, 2019 19:03
@utsavized utsavized requested a review from antross March 19, 2019 20:14
Copy link
Member

@antross antross left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me! 🎉

Thanks @utsavized for patiently keeping this PR up-to-date with our recent refactors.

@molant molant closed this in 12997ef Mar 19, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants