Skip to content

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

Merged
aeschli merged 6 commits intomicrosoft:masterfrom
pfongkye:fix/40149
Dec 20, 2017
Merged

Fix/40149: add data-* attributes autocomplete#17
aeschli merged 6 commits intomicrosoft:masterfrom
pfongkye:fix/40149

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