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
Move loading logic into each feature #1694
Conversation
👍 This is a great idea. |
08d2d09
to
4a7493e
Compare
Alright, big change. 🎉 I solved all the bugs I noticed for now, but I'm sure there's a few I didn't notice (out of the 83 javascript features 😰) Need some reviewers to try this together for a few days and then merging it asap, give the overarching patch. Pinging collaborators and recent contributors: @notlmn @jerone @dertieran @leggsimon @tanmayrajani @fxedel @yakov116 |
Everyone: Please don’t commit any changes to master until this PR is merged. |
This comment has been minimized.
This comment has been minimized.
@dertieran feature order could just be defined as For now, even the dependency thing sounds complicated (it must check the validity, remember the order, run when all the features are ready, and run at every |
This comment has been minimized.
This comment has been minimized.
safeOnAjaxedPages -> onAjaxedPages onAjaxedPages -> onAjaxedPagesRaw
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Co-Authored-By: bfred-it <github@bfred.it>
Co-Authored-By: bfred-it <github@bfred.it>
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
I finally had time to test it, and the features I use are still working. 👍 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Works well now 👌
I'll merge in ~14 hours to allow for more testing. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Works well here too
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Noticed a small edge case but not sure if it needs a fix. Out of curiousity though I noticed that refined-github only works if you’re signed in. Is that necessary? I assume this is because some features require authenticated API calls but I wonder whether the features that require a signed in user should define that in includes
eg.
features.add({
id: 'show-user-top-repositories',
include: [
features.isUserProfile,
+ features.userIsSignedIn
],
load: features.onAjaxedPages,
init
});
We're not interested in supporting logged-out state. Refined GitHub should not run at all if the user is logged out. |
🎉 Thanks all for the reviews! |
when
,where
) near the feature's code instead of being listed in 3 separate functions (init
,onDomReady
,ajaxedPagesHandler
) in a different file (content.js
)feature.add('feature-name')
on loadThis is just a proof of concept so only a feature has been implemented.