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

Rename the nodes field on browser/tab to node #1372

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Conversation

chrishtr
Copy link
Collaborator

...since it's just a root node.

A reader got confused about it since DocumentLayout's constructor takes a node but it looks like it is being passed nodes.

@pavpanchekha
Copy link
Collaborator

I'm not sure this change is an improvement.

I agree that nodes is not a good name, but node maybe isn't either? I think the name of the HTML and layout tree fields have to be considered together:

  • Currently self.nodes is the HTML tree and self.document is the layout tree.
  • From the POV of the Frame it's the whole tree, not a single node, an ideal name would reflect that.
  • In a real browser the "document" is the HTML tree.

Probably an idea naming scheme is, like, self.document for the HTML tree and self.layout for the layout tree? (I think I recall I didn't want to use layout as a field name because there's a method on layout objects called layout, but still seems better than what we have now.)

Anyway, unless there's a good reason to make this change I don't think it really helps consistency and is maybe more confusing (in that it's singular).

@chrishtr
Copy link
Collaborator Author

I agree that nodes is not a good name, but node maybe isn't either? I think the name of the HTML and layout tree fields have to be considered together:

How about self.html or self.html_tree? I think the main thing is to avoid a plural name, which makes it sound like a list.

@pavpanchekha
Copy link
Collaborator

Yes, agree on both counts. I think ideally we'd rename self.document, which is currently the layout tree, at the same time. self.html_tree and self.layout_tree work, self.document and self.layout also work, happy to pick either one.

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.

2 participants