Clean up onboarding code.#687
Conversation
poltak
left a comment
poltak
left a comment
There was a problem hiding this comment.
So much to go through, but most of the changes looks like good improvements on the previous code. Left some points on things I noticed that could maybe be better.
Another thing I noticed some console.log calls around the place that should be removed. Maybe do a search and remove those that relate to the overview and onboarding stuff.
| await utils.setOnboardingStage('annotation', nextStage) | ||
| // Close the curren select-option notification manually since | ||
| // accessing the toolbarNotification instance from here is not possible | ||
| document.querySelector('.memex-tooltip-notification').remove() |
There was a problem hiding this comment.
This seems to be a repeat of what's already in the destroyRootElement export from src/toolbar-notification/content_script/rendering module. Is it appropriate to use that here? (note that module seems to be content script, while this is main UI script)
There was a problem hiding this comment.
I didn't notice that export. These exports are imported in content-tooltip. Moved the code here from there because most of the code is onboarding logic.
| */ | ||
| export const setPowerSearchDone = () => async dispatch => { | ||
| await setLocalStorage(STORAGE_KEYS.onboardingDemo.step2, 'DONE') | ||
| await utils.setOnboardingStage('powerSearch', STAGES.done) |
There was a problem hiding this comment.
I can see the 'powerSearch' stage being set in lots of places using that string literal, along with stages like: annotation, tagging, backup. Does it make more sense to put those as part of the constants too?
| isPowerSearchChecked={powerSearchStage === 'DONE'} | ||
| isTaggingChecked={taggingStage === 'DONE'} | ||
| isBackupChecked={backupStage === 'DONE'} | ||
| isAnnotationChecked={this.props.annotationStage === 'DONE'} |
There was a problem hiding this comment.
Rather than checking these here, wouldn't it make more sense to do these in the selectors? You could export more selectors like isTaggingChecked which does this equality check for you
There was a problem hiding this comment.
Thanks. Added extra selectors for these! 👍
Also, added comments in a few files.
- Moved onboarding-notifications from src/content-script to src/overview/onboarding. - Moved onboarding-helper from src/popup to src/overview/onboarding.
- Simplified and cleaned up a lot of storage operations using abstracted helper functions defined in onboarding/utils.js. - Moved the string literals denoting the onboarding stages to constants - Added more comments. - Move non react stuff out of checklist container.
Also, removed stray console.logs.
Changed the string literals in all fetchOnboardingStage and setOnboardingStage calls.
digi0ps
force-pushed
the
patch/onboarding-cleanup
branch
from
e3893aa to
7d05c28
Compare
|
Tested it and all works. Except that I can't press the "go to dashboard" button on Firefox > in the first annotation success message. It does not work. Good job overall :) |
In the CongratsMessage button. Fixes issue where the button wasn't working in Firefox. Also, should fix the issue where this didn't work in content script.
|
Moved to using the remoteFunction So this should fix the bug which @oliversauter mentioned above and also the one in @MOHITSAKHUJA's branch. |
3 participants
This PR cleans up and refactors most of the onboarding code.
Written most of the stuff done in the commit messages, but I will summarise here.
content-tooltiporpopup. Moved those into the onboarding module itself and just importing in those places. Thepopup-helperonly contains a couple of functions, but have kept it in a separate file for neatness and separation of functionality.There is still some refactor needed with the
toolbar-notifications(string literals, same container for every notification making it tough to change styles), but that could be done in a separate PR.@oliversauter I've tested this thoroughly so that it doesn't break any existing stuff. It could also be useful if you test it from your end, whenever you're free, to see if I've missed anything.