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

Enforce node boundaries in places where it matters #53192

Merged
merged 6 commits into from
Jun 29, 2018

Conversation

JacksonKearl
Copy link
Contributor

@JacksonKearl JacksonKearl commented Jun 28, 2018

This just passes "false" explicitly in the places where the behaviour requires no node boundaries. If we were to now flip the default param to 'true', #53182 would be fixed.

@JacksonKearl JacksonKearl self-assigned this Jun 28, 2018
Explicitly set getNode's include boundries where needed
@ramya-rao-a
Copy link
Contributor

There are instances of getNode in updateImageSize.ts, mergeLines.ts and util.ts where the no value is passed for the include boundary parameter.
If we are changing defaults, then shouldn't these be updated as well?

I would say that we make this parameter mandatory instead of optional.
This way, when the getNode and getHtmlNode is called, we will know for sure whether we want the boundary included or not

@ramya-rao-a
Copy link
Contributor

On second thoughts, @JacksonKearl like you said, we can also look into including boundaries all the time.
It will need some code changes for the Select Next/Prev Item as well as Balance Out features.
But this would need a thorough testing of all the Emmet features

@JacksonKearl
Copy link
Contributor Author

Wait I don't think we need to include boundaries all the time (because of the cases like balance in/out and select next/previous). I just think we should flip the default value of the optional parameter to true, or even make it non-optional and change those cases which previously did not pass anything to pass true.

@ramya-rao-a ramya-rao-a merged commit 4f870af into microsoft:master Jun 29, 2018
@github-actions github-actions bot locked and limited conversation to collaborators Mar 27, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants