Skip to content

Fix/40149: add data-* attributes autocomplete #17

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

Merged
merged 6 commits into from
Dec 20, 2017

Conversation

pfongkye
Copy link

This issue is referenced here: microsoft/vscode#40149

@msftclas
Copy link

msftclas commented Dec 18, 2017

CLA assistant check
All CLA requirements met.

const dataAttr = 'data-';
let dataAttributes: string[] = [];

function addNodeDataAttributes(node: Node) {
Copy link
Author

Choose a reason for hiding this comment

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

I could try a non-recursive approach

let dataAttributes: string[] = [];

function addNodeDataAttributes(node: Node) {
node.attributeNames.filter(attr => attr.substr(0, dataAttr.length) === dataAttr).forEach(attr => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'd suggest doing a single forEach loop (no filter)
use startsWith from ../utils/strings for the check.

Copy link
Author

Choose a reason for hiding this comment

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

thanks for the review 👍


function addNodeDataAttributes(node: Node) {
node.attributeNames.filter(attr => attr.substr(0, dataAttr.length) === dataAttr).forEach(attr => {
if (dataAttributes.indexOf(attr) === -1) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Use a object or Map to check for extistece

if (!dataAttributes['attr']) {
    dataAttributes['attr'] = true;
    ...

Copy link
Author

Choose a reason for hiding this comment

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

ok I'll make the modification ASAP

@pfongkye
Copy link
Author

@aeschli requested modifications applied.

@aeschli aeschli added this to the January 2018 milestone Dec 20, 2017
@aeschli
Copy link
Collaborator

aeschli commented Dec 20, 2017

@pfongkye Thanks, great PR!

@aeschli aeschli merged commit 673b1e5 into microsoft:master Dec 20, 2017
@pfongkye pfongkye deleted the fix/40149 branch December 20, 2017 13:14
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.

3 participants