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

issue-74: Get first the attribute/value completions #78

Merged
merged 1 commit into from Aug 29, 2018

Conversation

Projects
None yet
3 participants
@abaracedo
Contributor

abaracedo commented Sep 29, 2017

Requirements

  • Filling out the template is required. Any pull request that does not include enough information to be reviewed in a timely manner may be closed at the maintainers' discretion.
  • All new code requires tests to ensure against regressions

Description of the Change

The options have to be selected by tag/attribute first because as says the comment in getAttributeValues:

# Some local attributes are valid for multiple tags but have different attribute values
# To differentiate them, they are identified in the completions file as tag/attribute

In #74 they reported that the rel attribute was not showing the stylesheet option. The problem was that the options given were for rel and not for link/rel.

I noticed that mistake doing a console.log of the returned values. First I console.log inside the method (mentioned before) the values returned by @completions.attributes["#{tag}/#{attribute}"]?.attribOption and the result was the expected, an array of length 13.

Then I console.log the values after get them and I saw that the values an array of length 14:

["alternate", "author", "bookmark", "help", "license", "next", "nofollow", "noreferrer", "prefetch", "prev", "search", "sidebar", "tag", "external"]

The result that was given it was for rel instead of link/rel.

Alternate Designs

None.

Benefits

It will display the correct suggestions for tag/attributes

Possible Drawbacks

None.

Applicable Issues

Fixes #74 and other suggestions for tag/attributes

@50Wliu

This comment has been minimized.

Show comment
Hide comment
@50Wliu

50Wliu Oct 2, 2017

Member

How about (pseudocode):

completions = []
completions.concat(/* tag/attribute */)
completions.concat(/* attribute */)
return completions
Member

50Wliu commented Oct 2, 2017

How about (pseudocode):

completions = []
completions.concat(/* tag/attribute */)
completions.concat(/* attribute */)
return completions
@abaracedo

This comment has been minimized.

Show comment
Hide comment
@abaracedo

abaracedo Oct 2, 2017

Contributor

@50Wliu What do you mean?

Contributor

abaracedo commented Oct 2, 2017

@50Wliu What do you mean?

@50Wliu

This comment has been minimized.

Show comment
Hide comment
@50Wliu

50Wliu Oct 2, 2017

Member

Instead of the ternary that we have now, I suggest splitting it up into shorter statements. So for example, line two of the pseudocode above would be:

completions.concat(@completions.attributes["#{tag}/#{attribute}"]?.attribOption)
Member

50Wliu commented Oct 2, 2017

Instead of the ternary that we have now, I suggest splitting it up into shorter statements. So for example, line two of the pseudocode above would be:

completions.concat(@completions.attributes["#{tag}/#{attribute}"]?.attribOption)
@abaracedo

This comment has been minimized.

Show comment
Hide comment
@abaracedo

abaracedo Oct 2, 2017

Contributor

I do not know a lot of Coffeescript, but trying to concatenate the resultant array of @completions.attributes["#{tag}/#{attribute}"]?.attribOption results in an empty array.

I tried with the two conditions but it remains empty. I am not sure if the problem could be for the variable that is used in the condition...

Contributor

abaracedo commented Oct 2, 2017

I do not know a lot of Coffeescript, but trying to concatenate the resultant array of @completions.attributes["#{tag}/#{attribute}"]?.attribOption results in an empty array.

I tried with the two conditions but it remains empty. I am not sure if the problem could be for the variable that is used in the condition...

@abaracedo

This comment has been minimized.

Show comment
Hide comment
@abaracedo

abaracedo Nov 21, 2017

Contributor

@50Wliu Can you give any hint to refactor it?

Contributor

abaracedo commented Nov 21, 2017

@50Wliu Can you give any hint to refactor it?

@50Wliu

This comment has been minimized.

Show comment
Hide comment
@50Wliu

50Wliu Nov 21, 2017

Member

Hi, sorry for the delay! I've been busy with other initiatives but will try to revisit this PR soon.

Member

50Wliu commented Nov 21, 2017

Hi, sorry for the delay! I've been busy with other initiatives but will try to revisit this PR soon.

@maxbrunsfeld maxbrunsfeld merged commit ab4c08c into atom:master Aug 29, 2018

2 checks passed

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

This comment has been minimized.

Show comment
Hide comment
@maxbrunsfeld

maxbrunsfeld Aug 29, 2018

Contributor

Thanks @abaracedo!

Contributor

maxbrunsfeld commented Aug 29, 2018

Thanks @abaracedo!

maxbrunsfeld added a commit that referenced this pull request Aug 29, 2018

maxbrunsfeld added a commit that referenced this pull request Aug 29, 2018

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