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

Separator extension improvements #1414

Closed

Conversation

puxlit
Copy link

@puxlit puxlit commented Sep 6, 2017

This is a rewrite of the separator extension. It features a more robust algorithm for placing the separator and tracking read posts (at the expense of bookkeeping). It also semi-supports tagged views.

Points of contention

  • The rewritten separator extension uses ES6 syntax and newish browser features (like Map and URLSearchParams), which'd leave users on older browsers in the lurch.

Checklist

  • Improve reconciliation algorithm to accommodate for posts that aren't arranged by strictly decreasing IDs; these posts typically appear to be published at the same time (at least to minute-level granularity)
  • Possible refactor once I eventually get around to adding separator support for channel views
  • Quick check for whether this extension conflicts with any of the other extensions
  • PR feedback
    • Documentation pass
  • Maybe add per-context reset control (so users needn't wipe the entire extension's settings)
  • Maybe add per-context enable/disable control (so users can opt in/out of placing separators on the dashboard or specific tags)
  • Maybe add visual indicators (so users know which posts have been witnessed, and roughly how far they are from the separator)

.eslintrc.yml Outdated Show resolved Hide resolved
// The workaround is to manually trigger a goal post reset (with the separator controls).
// Hints might be missing if we manually browse to a page’s URL.
// They might also _go_ missing should Tumblr change their URL patterns.
// - When the same context is open in multiple tabs, behaviour may seem unpredictable.
Copy link
Member

Choose a reason for hiding this comment

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

this comment should be jsdoc-style

Extensions/separator.js Outdated Show resolved Hide resolved
"use strict";

// Immutables
const DASHBOARD_PATH_REGEX = /^\/dashboard(?:\/(?:([2-9]|[1-9]\d+)\/(-?[1-9]\d*)\/?)?)?$/;
Copy link
Member

@nightpool nightpool Sep 6, 2017

Choose a reason for hiding this comment

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

is there a non-trivial functionality we gain by using [2-9]|[1-9]\d+? I understand that it's more correct, but \d+ seems like it would work just fine in the practical case and is much easier to read, leading to a more maintainable regex.

also when do we ever see negative post numbers?

Copy link
Author

Choose a reason for hiding this comment

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

¯\_(ツ)_/¯

I figured it was a common enough construct for matching against positive integers, and have a preference towards failing early. And the pattern is unlikely to change with frequency for maintainability to be an issue. (I mean, I feel the escapes and nested optional groups are what make this regex a pain to read.) But I can change it to \d+.

Negative post numbers are for backwards paging.

XKit.extensions.separator = (function() {
"use strict";

// Immutables
Copy link
Member

Choose a reason for hiding this comment

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

const/let, as well as the naming convention, makes this comment and the below (// Mutables) unnecessary and overly verbose.

@puxlit puxlit force-pushed the feature/separator-extension-improvements branch from 802a1c2 to 2de3a06 Compare September 30, 2017 20:09
@puxlit puxlit force-pushed the feature/separator-extension-improvements branch from 2de3a06 to 0698d3c Compare March 9, 2018 13:58
@puxlit puxlit force-pushed the feature/separator-extension-improvements branch from 0698d3c to cf128e7 Compare April 4, 2020 06:06
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