Why do you have to keep removing the class for these elements?

I tried to catch cases in which inactive deployments are reactivated again later. Don't if this happens.

Couldn't this be changed to .discussion-item .deployment-meta.is-inactive and skip checks inside loop?

If the last deployment message is invalid, I don't want to hide it. That's why I have to go through all deployments, and only hide them if they are invalid and not the last deployment.

I only meant to skip the isInactive check (by only looping through inactive deployment-items)!

Edit: In case removing classes is not needed (comment)

The isLastDeployment check refers to all deployments, inactive and active ones.

For example:

inactive -> hidden
inactive -> hidden
inactive -> hidden
active

inactive -> hidden
inactive

inactive -> hidden
inactive -> hidden
active
active

Not the last inactive should be shown, but the last deployment (even if it is inactive).

However, I could save the amount of deployment items and loop through the inactive ones then. Edit: I can't, as the index would be incorrect then.

Now I understand what you are trying to do. I thought the .is-inactive class was in .deployment-meta.

Can you try this:

const deployments = select.all('.discussion-item .deployment-meta');
deployments.pop(); // don't hide the last deployment, even if it is inactive

for (const deployment of deployments) {
	const discussionItem = deployment.closest('.discussion-item');
	const isInactiveDeployment = select.exists('.is-inactive', deployment);
 
	if (isInactiveDeployment) {
		discussionItem.setAttribute('data-rgh-inactive-deployment', 'true');
	}
}

`` -> ''

`` -> ''

This should be in the feature file. You should only have enableFeature(hideInactiveDeployments); here.

Use .dataset instead.

Use this:

Hides inactive deployments in the PR timeline

Is this all necessary? I think others observers on the discussion only listen to childList. Subtree is heavy

I'm not sure if the node list is updated when a deployment changes from active to inactive. That's why I'm watching class names.

I think you're still selecting them all, so whether the class is there or not will be found by is-inactive, right?

Worst case scenario there's an old inactive deployment on the current page... because the user never left the page. I'll take that over watching all nodes on PR pages.

Just tested it again at OpenLightingProject/open-fixture-library#449 with only childList set to true:

1. Initial timeline: (only commits and deployments)
   • 2 commits
   • Active deployment (with View deployment button)
2. Directly after pushed commit:
   • 2 commits
   • Active deployment (with View deployment button)
   • 1 commit
3. Some seconds later:
   • 2 commits
   • Active deployment (with View deployment button)
   • 1 commit
   • Pending deployment (with Pending label)
4. About a minute later:
   • 2 commits
   • 1 commit
   • Active deployment (with View deployment button)

I thought that at least subtree had to be turned on, as from 3. to 4., no element is (visually) added or removed from the timeline. However, it seems like that not the last deployment's contents are changed from pending to active, but the complete timeline item is replaced with a new node.

So you're right, childList is sufficient (as it's the default, I'll remove the whole options object).

This can just be a class

As I'm watching class names, setting a class would create an infinite loop.

Edit: I'm not watching them anymore, so I'll use a class instead.

No need for space here

This can be if select.exists without the extra var. Same for discussionItem on the next line

Deployments only appear in PRs, right? Then before observeEl I think we need this:

if (!pageDetect.isPR()) {
	return;
}

I thought this is done in content.js, but I added it anyways.

We’re trying to move away from that, so that the feature is self-contained