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.

Sign up for GitHub

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

Jump to bottom

[core] Add tests for column selector #845

Merged
merged 7 commits into from
Jan 13, 2021
Merged

[core] Add tests for column selector #845

merged 7 commits into from
Jan 13, 2021

Conversation

DanailH
Copy link
Member

@DanailH DanailH commented Jan 12, 2021 •
edited
Loading

Fixes #345

Part 2 of "Implement Column selector" - tests

@DanailH DanailH added component: data grid This is the name of the generic UI component, not the React module! new feature New feature or request labels Jan 12, 2021
@DanailH DanailH self-assigned this Jan 12, 2021
@DanailH DanailH changed the title Feature/data grid 345 add tests column selector [DataGrid] Add tests for column selector Jan 12, 2021
@DanailH DanailH requested a review from dtassone January 12, 2021 15:13
oliviertassinari
oliviertassinari reviewed Jan 12, 2021
View reviewed changes
packages/grid/data-grid/src/tests/toolbar.DataGrid.test.tsx Outdated
expect(getColumnHeaders()).to.deep.equal(['id', 'brand']);

fireEvent.click(getByText('Columns'));
fireEvent.click(document.querySelectorAll('[role="tooltip"] [name="id"]')[0]);
Copy link
Member

@oliviertassinari oliviertassinari Jan 12, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

A related not. Is it expected to have a role tooltip on the Popper we display? We don't have any documentation about the accessibility on the component's page https://next.material-ui.com/components/popper/. I wonder if it would make sense to add one cc @eps1lon.

Copy link
Member

@oliviertassinari oliviertassinari Jan 12, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

@DanailH Why are we using querySelectorAll over querySelector if we only query one element?

Copy link
Member Author

Choose a reason for hiding this comment

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

@DanailH Why are we using querySelectorAll over querySelector if we only query one element?

That's my bad, I must have been testing different things and forgot about that, I'll fix it.

Regarding the Popper missing a role -> that would have been my prefered method of selecting it

Copy link
Member

Choose a reason for hiding this comment

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

One would have to look into whether this makes sense. For example, https://next.material-ui.com/components/popper/#scroll-playground probably is not a tooltip. It would be confusing to have a Popper and Tooltip when both have role="tooltip"

@oliviertassinari
Copy link
Member

oliviertassinari commented Jan 12, 2021 •
edited
Loading

Regarding the requirements for closing #345.

⚠️ The linked issue in the description is not the right one.

I wonder, should we mention disableColumnSelector in the documentation section we have recently added? Or even better, should we reverse the flag to enableColumnSelector? I'm asking because if a developee renders the data grid without a toolbar, a end user has no way to add columns once he hides them with the column menu.

@DanailH
Copy link
Member Author

DanailH commented Jan 12, 2021

@oliviertassinari , yes I think we should mention it. I'll add it as part of this PR. Regarding whether we should call it disableColumnSelector or 'enableColumnSelector' and hide it by default needs to be applied for the rest of the toolbar elements.

@oliviertassinari
Copy link
Member

oliviertassinari commented Jan 12, 2021 •
edited
Loading

Regarding whether we should call it disableColumnSelector or enableColumnSelector

Sorry, I shouldn't have used the name of the prop to explain what I was wondering. The name of the boolean are meant to always have false as the default value, it's a constraint we enforce in all the components.
My question was really about should the "hide column" menu item in the menu column be opt-in or opt-out?

@DanailH
Copy link
Member Author

DanailH commented Jan 12, 2021

No, I understood the question, maybe I didn't really explain my point the correct way - I don't think it matters much now, we are about to rework the way the toolbar works. That is one side of the problem. When we do that means that basically, everything in the toolbar will be opt-in so it makes sense to have the booleans that are related to matching toolbar features as enable*.

@oliviertassinari
Copy link
Member

Sounds great

@DanailH
Copy link
Member Author

DanailH commented Jan 13, 2021

@dtassone can I merge this?

@DanailH DanailH merged commit 093b125 into mui:master Jan 13, 2021
@oliviertassinari oliviertassinari changed the title [DataGrid] Add tests for column selector [core] Add tests for column selector Jan 16, 2021
@oliviertassinari oliviertassinari added test and removed new feature New feature or request labels Jan 16, 2021
dtassone pushed a commit to dtassone/material-ui-x that referenced this pull request Jan 19, 2021
@DanailH @dtassone
[DataGrid] Add tests for column selector (mui#845)
bf5bb2b
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@eps1lon eps1lon eps1lon left review comments

@dtassone dtassone dtassone approved these changes

@oliviertassinari oliviertassinari oliviertassinari approved these changes

Assignees

@DanailH DanailH

Labels
component: data grid This is the name of the generic UI component, not the React module! test
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[DataGrid] Implement Column selector
4 participants
@DanailH @oliviertassinari @dtassone @eps1lon