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

Hotfix/1.2.2 #841

Merged
merged 7 commits into from
Jun 21, 2019
Merged

Hotfix/1.2.2 #841

merged 7 commits into from
Jun 21, 2019

Conversation

poltak
Copy link
Member

@poltak poltak commented Jun 19, 2019

Should fix #840. That was triggered by the recent change to when we load the content script: now at document end rather than start.
That's fine and easy to fix, but I can't remember why exactly we needed to change that though it's something to do with the twitter stuff. I looked at the commit history for it and found the commit that changed that (0a15e5b), although that also didn't have any details about the change.

It's not a big problem, but a good suggestion for future commits is to just put a brief sentence or two detailing the changes if they're not entirely obvious from the code. Try to think about if you looked at that commit weeks/months later, could you (and others) work out what the intention was from the code diff + commit message supplied?
The more self-documenting our commit log is, the less following up, or searching through notion/GH/slack threads we might need to do with people. No need to spend much time on them though.

- data indexed depends on user indexing pref
- I don't completely understand why it wasn't being caught before
- it seems to be now though
- previous implementation assumed content script was loaded at document start, running the main logic on `DOMContentLoaded` event
- we recently changed the content script to load at document end, hence that event never gets triggered
- now it waits on a promise which resolves when `document.readyState` is `complete`, which seems to work
- should fix #840
@blackforestboi
Copy link
Member

I think the original problem was that it prevented the twitter integration icon to be properly loaded, but that works.
Also the capturing of the ID works again and could be merged. Can you also get @poltak this one into here? I thought this was already merged. Weird.
#830

# Conflicts:
#	src/activity-logger/background/tab-change-listeners.ts
@blackforestboi blackforestboi merged commit c363982 into master Jun 21, 2019
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