Repository navigation
fix(rux-tree-node): emit ruxtreenodeselected once when selected via setSelected - #1527
Closed
East Agile Tracker (eastagiletracker) wants to merge 1 commit into
Conversation
…etSelected The selected watcher called setSelected, which emitted again after the watcher-triggered call had already emitted, so selecting an unselected node through setSelected(true) or the Enter key fired the event twice. The watcher now emits on change, and setSelected only emits itself when re-selecting an already selected node.
🦋 Changeset detectedLatest commit: b80e9a7 The changes in this PR will be included in the next version bump. Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
✅ Deploy Preview for astro-preview ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
✅ Deploy Preview for astro-stencil ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR proposes fixing
rux-tree-nodeemittingruxtreenodeselectedtwice when a node is selected throughsetSelected(true). Fixes #1458. We include this PR work along with a full history of your repo at https://eastagiletracker.com/projects/652. You can sign in with your GitHub ID to claim ownership of the project.What changed and why
In
rux-tree-node.tsx,setSelected(true)setsthis.selected, which synchronously fires the@Watch('selected')handler. That handler calledsetSelected(newValue)again, which emittedruxtreenodeselected, and then the original call emitted it a second time. Clicking a node only emitted once because the click handler setsselecteddirectly and goes through the watcher a single time. The Enter key handler callssetSelected(true)too, so keyboard selection had the same double emit. The fix moves the emit into the watcher (so every change totrueemits exactly once, whether it comes from a click, the Enter key,setSelected, or setting theselectedprop), andsetSelectednow only emits on its own when re-selecting a node that is already selected, which keeps the existing single event for that case. Deselecting still emits nothing. A patch changeset is included.Reproduction at current
main(6f4dc9f). With the new regression tests added tosrc/components/rux-tree-node/test/tree-node.spec.tsand the component unchanged,npx playwright test --project=chromium-e2e src/components/rux-tree-nodefails 3 tests:setSelected(true)on an unselected node,setSelected(true)on a node inside arux-tree, and selecting with the Enter key, each withError: expected event "ruxtreenodeselected" to have been called 1 times, but was called 2 times.Verification. After
stencil build, the fullchromium-e2eproject was run before and after the change: 334 passed / 8 failed onmain, 340 passed / 8 failed with this branch (the 6 new tests all pass). The 8 failures are identical in both runs and unrelated to the tree (rux-dialog,rux-tabs,rux-textarea,rux-timeline). The new tests also pin the paths whose behavior should not move: setting theselectedprop still emits once,setSelected(true)on an already selected node still emits once, andsetSelected(false)emits nothing. The existingrux-treetests (keyboard controls, independent selection across trees) stay green, and therux-tree-node.spec.tsxunit spec passes.How this was managed
This work was tracked as story https://eastagiletracker.com/projects/652/stories/566812 on a board imported from this repo's issues and pull requests (1,453 stories), which we used to manage the fix: https://eastagiletracker.com/projects/652
If you'd rather not receive contributions like this, reply
no-more-prson this pull request and we won't open any further ones on your repositories.Lawrence W. Sinclair
CEO / East Agile
linkedin.com/in/lwsinclair/
eastagile.com