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

fix observation of page title mutation #13

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

Conversation

mixmix
Copy link
Contributor

@mixmix mixmix commented Mar 11, 2018

Type: Patch

Problem: I previously introduced a bug into hypertabs where I added another level of wrapping to pages when they were added. This then had the MutationObserver watching for title changes on the wrapper instead of the content.

Solultion: add a MutationObserver which targets the right thing

@dominictarr
Copy link
Collaborator

hmm, I'm not sure wether this is the best solution. I think adding the wrapper broke not only title, but also the other feature (notifications, etc) that were based on observing the page. Since the page is now really the first child of the page wrapper, won't these be broken?

If these features are dead code that no one is using we should delete that code.
But really, I think this module needs tests (given patchbay and patchless both depend on it)

@mixmix
Copy link
Contributor Author

mixmix commented Mar 14, 2018

seems like 2 possible paths:

  • merge this for now as a quick patch which gets titles working. rework whole thing later
  • just close this and wait for rework

I think there are a few things I'd like to change about this module, and I'm sure you've got a list you've been collecting. Ah I see in other PR you agree and we should collect.

I probably need to think about this more and read what you've done with patchless more closely to try and understand the IDEA. I've got a bunch of opinions that I've been developing through building scuttle-poll and similar which I'd like to merge with your thinking sometime

@dominictarr
Copy link
Collaborator

I enough things depend on this that I want to be really careful about merging, especially since we don't have tests. if we had tests I'd be way more inclined to merge, because the tests would have caught things such as the titles. this is complicated enough that it needs tests, but currently in the uncanny valley where it works, but without tests.

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