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

Add Context module #1052

Merged
merged 3 commits into from
May 14, 2014
Merged

Add Context module #1052

merged 3 commits into from
May 14, 2014

Conversation

matheod
Copy link
Collaborator

@matheod matheod commented May 13, 2014

Fix #437
Fix #291

}
},
setDefaultContext: function() {
$(".bylink").each(function() {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@honestbleeps think it's worth using RESUtils.forEachChunked?

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, anything running on all comments should use chunking or UI will freeze on large pages

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

well, this function is only called on inbox. so max .. 100? but it wouldn't hurt to chunk

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah my bad.. max 100 is probably fine to leave as-is

On Tue, May 13, 2014 at 4:39 PM, Andy Tuba notifications@github.com wrote:

In lib/modules/context.js:

  •       }
    
  •       if ((RESUtils.pageType() === 'inbox') && !isNaN(this.options.defaultContext.value) && this.options.defaultContext.
    
    value >= 0) {
  •           this.setDefaultContext();
    
  •       }
    
  •   }
    
  • },
  • addViewFullContext: function() {
  •   if(location.search !== "?context=10000") {
    
  •       var pInfobar = document.querySelector(".infobar:not(#searchexpando) p");
    
  •       if(pInfobar) {
    
  •           pInfobar.innerHTML += '&nbsp;<a href="?context=10000">view the full context</a>&nbsp;→';
    
  •       }
    
  •   }
    
  • },
  • setDefaultContext: function() {
  •   $(".bylink").each(function() {
    

well, this function is only called on inbox. so max .. 100? but it
wouldn't hurt to chunk


Reply to this email directly or view it on GitHubhttps://github.com//pull/1052/files#r12610307
.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(note for posterity) plus it's just changing the href attribute, so not serious DOM manipulation / repaints

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(note for posterity) only 1ms for 100 context link

honestbleeps added a commit that referenced this pull request May 14, 2014
@honestbleeps honestbleeps merged commit ea18690 into honestbleeps:master May 14, 2014
@jewel-andraia jewel-andraia mentioned this pull request May 18, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

add context button to permalinked comments Override ?context=# with user option
3 participants