with within specified from and to are only looked for in within#61
Open
kfritsch wants to merge 1 commit intokdeloach:masterfrom
Open
with within specified from and to are only looked for in within#61kfritsch wants to merge 1 commit intokdeloach:masterfrom
kfritsch wants to merge 1 commit intokdeloach:masterfrom
Conversation
kdeloach
reviewed
Dec 16, 2021
Owner
kdeloach
left a comment
kdeloach
left a comment
There was a problem hiding this comment.
This is a good idea. But I'm not familiar enough with the within feature to merge this yet.
What this could use is a demo to showcase how this works and what problem it solves. The demo would also help to verify if this is a breaking change or not so I could update the package version accordingly.
|
|
||
| findElementWithin(className, within) { | ||
| return within.querySelector(`.${CSS.escape(className)}`); | ||
| } |
Owner
There was a problem hiding this comment.
This seems to be more or less equivalent to the existing findElement method. Maybe we could add an optional param to the original method instead?
findElement(className, parent=document) {
return parent.getElementsByClassName(className)[0];
}| offsetY -= | ||
| parentBox.top + | ||
| (window.pageYOffset || document.documentElement.scrollTop) - | ||
| parent.scrollTop; |
Owner
There was a problem hiding this comment.
The formatting of these lines seems off.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Feature Request
I would find it more intuitive if the from and to elements were searched exclusively within the within element if that prop is specified in the component LineTo. This would make specifying the selector classnames a little easier, when the same element occurs multiple time within the dom. This would NOT be a breaking change. From and to have to be within this within element in order to be placed relative to that element, since the placement is computed with regards to the within element.
I would also find it more intuitive to be able to pass any css selector to from, to and within and use querySelector to find the element. Being able to pass an id to identify an element within the dom would make a lot of sense after all. However this would be a breaking change so I did not add this with this pr.