Closed marcustyphoon closed 5 days ago
Aside: here's a fun use of the fairly-new groupBy
global, for... well, "performance", in theory. Why process each timeline element for every post that gets added instead of processing each one once?
Well, because performing a simple regex, even from a string, is so fast it doesn't show up in the chromium profiler, and this code is way too long (Map
s are so clunky) and requires us to bump minimum browser versions, is why. Not worth it.
if (timeline) {
const timelineFilters = [timeline].flat().filter(Boolean);
const postsByTimeline = Map.groupBy(
postElements,
postElement => postElement.closest(timelineSelector)
);
const filtered = [...postsByTimeline.entries()]
.map(([timelineElement, postElements]) =>
timelineElement &&
timelineFilters.some(timelineFilter => timelineFilter(timelineElement))
? postElements
: []
)
.flat();
postElements = filtered;
}
getTimelineItemWrapper
when used in carousel hiding code.Haven't! Will do that.
Also, is there anything else you want me to look at before putting together the v1.0.0 release?
I do have a category of small diffs that fix regressions or potential pitfalls that I use the bugfix
label for (that could also include #1486, I guess). Could take a glance at those and see if any look important to prioritize to you!
Description
Tumblr has a new internal
TimelineV2
component that changes some attributes we use to detect timelines and which posts a timeline is on. There is currently an incomplete rollout of this migration, partially behind some feature flags.This:
filterPostElements
function'stimeline
option field to take (an) arbitrary function(s) rather than a regular expression. This allows us to update the selector logic without grepping scripts, and using a function as the API lets us use complex behavior likematches()
if necessary to deal with edge cases during migration or that result from redpop bugs. Bikeshed away.data-timeline-id
attribute for all of the timelines we currently use which at time of updating this description either use the new code or do so if forced with flags... except Tumblr Patio columns, for which I'm waiting on tweaks to the ids of since they're too ambiguous at the moment.getTimelineItemWrapper
on V2 timelines with endless scrolling off, on which thecell
element has no classname; see the linked issue.I experimented with a) generating timeline filter functions and timeline css selectors automatically from the same source data and b) making that source data exhaustive in #1485. It was interesting but kind of bad. It does make this diff look extremely boilerplate-y by comparison, though.
Resolves #1488. Resolves #1480.
Testing steps
If I've left this line in the PR description I haven't exhaustively tested this yet; keep that in mind.
Tests should include having endless scrolling on/off, and having feature flags on/off. I will not post my method for toggling feature flags in production here but it's very easy.
I have also been running a user style to make things quick and easy: