Task/pack file restructure part I #377
Conversation
|
Looks like even just this minimal file restructure requires refactoring of code as I mentioned in #373 (comment). |
nickytonline
changed the title
| } | ||
|
|
||
| function renderPage() { | ||
| import('../src/Onboarding') |
There was a problem hiding this comment.
@benhalpern, you'll like this performance tweak. We only load onboarding if the user is signed in and hasn't previously seen the onboarding. Before, the component was imported all the time.
There was a problem hiding this comment.
I do. I'm also excited to improve on several fronts here.
| ) | ||
| .catch(error => { | ||
| // eslint-disable-next-line no-console | ||
| console.error('Unable to load onboarding', error); |
There was a problem hiding this comment.
I see that there is airbrake for the back-end, but from what I can tell, we don't use any client-side error reporting services. Consider a package for handling client-side errors as we'll creating more and more in the front-end now.
| @@ -1,71 +0,0 @@ | |||
| import { h, render } from 'preact'; | |||
There was a problem hiding this comment.
This has been renamed from chat.jsx to Chat.jsx as per eslint rules.
| @@ -1,40 +0,0 @@ | |||
| import { h, render } from 'preact'; | |||
There was a problem hiding this comment.
This has been renamed from pack.jsx to Onboarding.jsx as that is really what this pack file is for.
| //Not yet ready | ||
| if (!window.location.href.includes('ask-for-notifications')){ | ||
| return "dont-ask" | ||
| // Not yet ready |
There was a problem hiding this comment.
A little note for whoever comes across this PR, there are only three permissions available. See https://developer.mozilla.org/en-US/docs/Web/API/Notification/permission
|
|
||
| return permission === 'default' ? 'waiting-permission' : permission; |
There was a problem hiding this comment.
Since there are only three possible permission types, https://developer.mozilla.org/en-US/docs/Web/API/Notification/permission, I'm assuming when it's 'default' @benhalpern, you wish it to be set to 'waiting-permission'?
nickytonline
changed the title
|
As @rhymes mentioned in another issue/PR, there are vulnerablilities detected which makes CI crap out. |
| navigator.serviceWorker.ready.then((serviceWorkerRegistration) => { | ||
| serviceWorkerRegistration.pushManager.getSubscription() | ||
| .then(function(subscription) { | ||
| navigator.serviceWorker.ready.then(serviceWorkerRegistration => { |
There was a problem hiding this comment.
prettier in action here.
| getUserDataAndCsrfToken() | ||
| .then(loadChat) | ||
| .catch(error => { | ||
| // eslint-disable-next-line no-console |
There was a problem hiding this comment.
As mentioned elsewhere, client-side error reporting should be put in place.
nickytonline
changed the title
|
@maestromac, @benhalpern or the Internet, this is ready for a review. |
|
@maestromac, code climate was all fixed but since I pushed my last commit (just tests), now 38 more issues popped up. Given that this PR really was just to move some files, I think the additional issues to fix should come in another PR, especially since all the new code climate issues that just appeared after my last commit (just tests) are not even in the files I changed. Happy to fix them in another PR when those files actually get touched. |
|
Awesome. Will give this full attention tomorrow @nickytonline. I agree with the assessment on linting. Outside of this scope. |
| } | ||
|
|
||
| totalTimeWaiting += waitTime; | ||
| setTimeout(waitingOnUserData, waitTime); |
There was a problem hiding this comment.
The previous implementation was using setInterval which could have bogged things down as setInterval just fires after n ms. regardless of whether or not what's in it is completed. With setTimeout things can run within it and once they're complete it can be called again. Having said that, this still doesn't seem like the best approach, but the current implementation is looking for DOM attributes being updated.
MutationObserver is an option as you can configure it to only listen for specific attributes' changes, but I think there is probably a better approach than setting these values on DOM attributes. However, since this PR was initally just to rename and move some files, it seems out of scope of this PR.
| @@ -0,0 +1,109 @@ | |||
| import { getUserDataAndCsrfToken } from '../util'; | |||
There was a problem hiding this comment.
Just a note that these tests won't appear in the coverage report for the moment. As soon as the chat folder is within the src folder, they'll appear in coverage reports. This will happen in another PR.
There was a problem hiding this comment.
This all looks good to me, but @maestromac can you give it a once-over?
| } | ||
|
|
||
| function renderPage() { | ||
| import('../src/Onboarding') |
There was a problem hiding this comment.
I do. I'm also excited to improve on several fronts here.
|
|
||
| return permission === 'default' ? 'waiting-permission' : permission; |
What type of PR is this? (check all applicable)
Description
Part of the work for #327. Casing of JSX files have been made uniform to follow PascalCasing. See https://github.com/airbnb/javascript/tree/master/react#naming. Also the onboarding entry point is now name properly, pack.js -> Onboarding.jsx
Added to documentation?
[optional] What gif best describes this PR or how it makes you feel?