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

highlightAll accepting an element to query #397

Closed
wants to merge 1 commit into from

Conversation

itsadrake
Copy link

Currently, the highlightAll function queries document for all highlightable elements, then calls highlightElement on all of them. Sometimes we may wish to do so for a specific element deeper down the DOM tree.
There is already a highlightElement function, but it exits for elements that are not immediately highlightable. The highlightAll function seems to be a good candidate for a simple generalisation.
If no element is passed, the function can default to querying document, hopefully requiring minimal changes to existing code.

If no element is passed, the function defaults to querying document, hopefully requiring minimal changes to existing code.
@apfelbox
Copy link
Contributor

This is a BC break. If you add the parameter as the last one, it would be BC safe. Also, I would name the parameter rootElement.

@itsadrake
Copy link
Author

That would make the argument order inconsistent with that of highlightElement. Perhaps we could avoid breakage by introducing a new function altogether? Perhaps one called highlightChildren or some such.

@apfelbox
Copy link
Contributor

Ah, you are right.

I could live with a new function. Maybe something more modern (promises, anyone)? So that we could slowly move all code to the new functions - this one could automatically and intelligently handle both a single element or a set of elements.

@LeaVerou
Copy link
Member

Um, not sure I follow why highlightElement doesn’t suffice in this case?

@itsadrake
Copy link
Author

Because highlightElement does not highlight any of its children, and it would be nice if we could query a node other than document.

tanakahisateru added a commit to tanakahisateru/prism that referenced this pull request Nov 22, 2017
tanakahisateru added a commit to tanakahisateru/prism that referenced this pull request Nov 22, 2017
tanakahisateru added a commit to tanakahisateru/prism that referenced this pull request Nov 22, 2017
@tanakahisateru
Copy link
Contributor

I created another pull-request #1230

Golmote pushed a commit that referenced this pull request Dec 5, 2017
@Golmote
Copy link
Contributor

Golmote commented Dec 5, 2017

#1230 fixed this issue in a more compatible way.

@Golmote Golmote closed this Dec 5, 2017
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.

5 participants