Restore right-clicked class on a right-clicked tab#368
Conversation
Used by some packages to identify the tab from which a command was invoked.
|
Interesting that the Atom team removed it because it seemed to be interfering with other things Can you confirm that it doesn't interfere with the issue that the Atom team marked as fixed? |
|
Not sure what you mean. If you mean the focus issue described in atom/tabs#531, I infer from context that the removal of |
|
Also, now that I look more closely, atom/tree-view#1205 is what we'd want to do long-term, assuming I'm reading it correctly. The invoked command would be able to consult We'd still want this PR in the short term, though, because it doesn't require affected packages to push an update just to fix the regression. |
DeeDeeG
left a comment
DeeDeeG
left a comment
There was a problem hiding this comment.
I'm not deeply familiar with how the right clicking logic is meant to work in the package ecosystem/around tabs, but I'll approve to indicate I have faith this is doing what it is intending to do.
Especially with the included tests passing in CI. (Test includes two positive "right-clicked" cases, and a negative "not right-clicked" (or more precisely a "not the most recently right-clicked tab" case), so the test scenario overall is somewhat unlikely to give false positives/false negatives.)
(Maybe @pulsar-edit/core have insight that this has only the intended effect and no unintended side effects? Would not mind a second opinion here from someone who's more familiar with this.)
| describe("when the tab bar is right-clicked", () => { | ||
| it("adds the right-clicked class when right-clicked", () => { | ||
| triggerClickEvent(tabBar.tabAtIndex(0).element, {button: 2}); | ||
| expect(tabBar.tabAtIndex(0).element.classList.contains('right-clicked')).toBe(true); | ||
| triggerClickEvent(tabBar.tabAtIndex(2).element, {button: 2}); | ||
| expect(tabBar.tabAtIndex(2).element.classList.contains('right-clicked')).toBe(true); | ||
| expect(tabBar.tabAtIndex(0).element.classList.contains('right-clicked')).toBe(false); | ||
| }); | ||
| }); | ||
|
|
|
Thank you for the contribution, @savetheclocktower! |
4 participants
Fixes #349.
Identify the Bug
Until recently, when a tab was right-clicked, the
tabspackage marked it with aright-clickedclass. As described in #349, this PR removed that functionality because of a belief that it was unused.At least one popular package, Zentabs, was broken by this change — when a user right-clicked a tab and selected Pin Tab, Zentabs used the
right-clickedclass to determine which tab the command was invoked against.Description of the Change
The code being restored is exactly equivalent to what was removed in the aforementioned PR.
Alternate Designs
Because this was restoring code that used to be present, I didn’t think much about alternate approaches, but they can be explored once the regression is fixed.
Possible Drawbacks
It occurs to me that, if you right-click on a tab and then exit the context menu, that tab will continue to bear the
right-clickedclass name until you right-click on a different tab, but I think that’s OK.Verification Process
All existing tab specs pass. The new spec I wrote fails on
masterand passes on this PR branch.By pinning this package in dev mode, I have ascertained that this fixes the Zentabs issue. I am once again able to pin a tab by right-clicking on it and selecting the Pin Tab option.
Release Notes
Restores the ability of a package to know when a given tab was right-clicked on.