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

[TreeView] Test current behavior of active item removal #21720

Merged
merged 2 commits into from Jul 27, 2020

Conversation

eps1lon
Copy link
Member

@eps1lon eps1lon commented Jul 8, 2020

Adds a test for #20204 which was closed in #21695 but no test was added as far as I can tell.

The test is especially interesting since the behavior is novel: Removing the focused item moves focus to another item of the widget. In other widgets we let focus return to document.body.

@eps1lon eps1lon added bug 🐛 Something doesn't work component: tree view TreeView, TreeItem. This is the name of the generic UI component, not the React module! labels Jul 8, 2020
@eps1lon eps1lon changed the title Fix/tree view/tabbable removal [TreeView] Fix no tabbable tree item after focused removal Jul 8, 2020
@mui-pr-bot
Copy link

mui-pr-bot commented Jul 8, 2020

Details of bundle changes

Generated by 🚫 dangerJS against 6a2b3f1

@eps1lon
Copy link
Member Author

eps1lon commented Jul 9, 2020

I think I'm waiting for #21695 first. We should simplify this logic even more so that we can't even make mistakes in the reducer (I already did).

We only need to track which node is active. Whether it is "focused" is derived from the active node and whether the tree is focused. No need to track the id in two variables.

@eps1lon eps1lon marked this pull request as draft July 14, 2020 12:09
@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Jul 25, 2020
@oliviertassinari
Copy link
Member

@eps1lon The linked pull requests and issues have been closed, is the effort still relevant?

@eps1lon
Copy link
Member Author

eps1lon commented Jul 27, 2020

@eps1lon The linked pull requests and issues have been closed, is the effort still relevant?

Yes, see

I think I'm waiting for #21695 first.

@eps1lon eps1lon changed the title [TreeView] Fix no tabbable tree item after focused removal [TreeView] Test current behavior of active item removal Jul 27, 2020
@eps1lon eps1lon added test and removed PR: out-of-date The pull request has merge conflicts and can't be merged bug 🐛 Something doesn't work labels Jul 27, 2020
@eps1lon eps1lon force-pushed the fix/TreeView/tabbable-removal branch from 7440f3a to 6a2b3f1 Compare July 27, 2020 19:16
@@ -32,6 +33,10 @@ describe('<TreeItem />', () => {
}));

describe('warnings', () => {
beforeEach(() => {
PropTypes.resetWarningCache();
Copy link
Member Author

Choose a reason for hiding this comment

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

Cleanup of #21695 which added a test that would fail in in watchmode on subsequent runs.

@eps1lon eps1lon requested a review from joshwooding July 27, 2020 19:18
@eps1lon
Copy link
Member Author

eps1lon commented Jul 27, 2020

Updated test. Please also read the updated PR description.

@eps1lon eps1lon marked this pull request as ready for review July 27, 2020 19:19
@joshwooding joshwooding merged commit 5126864 into mui:next Jul 27, 2020
@eps1lon eps1lon deleted the fix/TreeView/tabbable-removal branch August 7, 2020 18:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: tree view TreeView, TreeItem. This is the name of the generic UI component, not the React module! test
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants