-
-
Notifications
You must be signed in to change notification settings - Fork 155
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
add Floki.find_by_id/2 #548
Conversation
Maybe we should also only return nil or the first element found? That would mirror the behavior of |
I think returning the first element is the best option, in the future we could implement an optimization to stop the traversal on the first matching node, and has the same behaviour as |
@SteffenDE yeah, I agree. We don't need to return a list here.
I believe if we document well, it's OK to use |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Only minor points, but LGTM!
What about calling it “get” to mirror browsers then? Or get_by_id? |
I like the idea of using |
@philss yep, I like it too. Changed it. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🚢
@SteffenDE thank you for the contribution! and thanks @ypconstante and @josevalim for the points and the discussion! |
Related to the problem and discussion in phoenixframework/phoenix_live_view#3144.
In LiveViewTest we need to find elements by their id. Currently, we use
Floki.find(html, "##{id}")
, but this breaks as soon as there are special characters in the id. We could useFloki.find(html, %Floki.Selector{id: id})
, but theFloki.Selector
struct is not documented and therefore I'm not sure if it's something to rely on.As
document.getElementById
is also a frequently used API in the browser, adding afind_by_id
or similar function to Floki could be quite useful.This PR adds a simple find_by_id function that internally uses the
%Floki.Selector{id: id}
struct.As using find with a string is deprecated, I did not add this functionality to find_by_id.