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

Less wrapper elements #18

Conversation

flash1293
Copy link

@flash1293 flash1293 commented Jan 18, 2021

Based on #17

This PR reduces the number of wrapper elements per cell - as long as the cell is not hovered, there's just the regular cell element with one wrapper plus a hidden paragraph for screen reader a11y. This one last wrapper element is still in place because the CSS got pretty ugly otherwise and it didn't seem to have too much influence on performance.

This also prevents a double-render by waiting for the header row to get a height first before placing the data cells.

This also fixes a bug with onMouseLeave which is not reliable, this PR solves it via css to only show the buttons in the cell is actually hovered.

Copy link
Owner

@chandlerprall chandlerprall left a comment

Choose a reason for hiding this comment

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

Wow, this is impressively faster. Thank you for looking into all of this!

Cell action should appear immediately on click

One thing we decided with cell actions is that they should appear immediately onFocus instead of being delayed like the popover button

Icon animations reset when expanding the popover

When clicking on the popover button, the cell actions hide and then re-animate in, they should stay present. This may be fixed for free with the previous item.

animation_reset.mov

src/components/datagrid/data_grid.test.tsx Outdated Show resolved Hide resolved
src/components/datagrid/data_grid_cell.tsx Outdated Show resolved Hide resolved
src/components/datagrid/data_grid_cell.tsx Show resolved Hide resolved
src/components/datagrid/data_grid_body.tsx Outdated Show resolved Hide resolved
src/components/datagrid/data_grid_cell.tsx Outdated Show resolved Hide resolved
src/components/datagrid/data_grid_cell.tsx Outdated Show resolved Hide resolved
src/components/datagrid/data_grid_cell.tsx Show resolved Hide resolved
@chandlerprall
Copy link
Owner

Quick testing pass on the Data grid focus page:

  • "When removing focus from the grid and then returning, the last focused cell remains focused." is no longer working
  • "Clicking on an interactive element within a cell the focus should always remain on that element, not shift to the cell or another element unless a subsequent user action changes it." does not work if the column is expandable
  • Not discussed in a rule, but the popover for the expandable + 2 interactives column doesn't have a focus trap.

@flash1293
Copy link
Author

Thanks for the detailed look @chandlerprall

I addressed some issues, I'm not sure about these two:

"Clicking on an interactive element within a cell the focus should always remain on that element, not shift to the cell or another element unless a subsequent user action changes it." does not work if the column is expandable
Not discussed in a rule, but the popover for the expandable + 2 interactives column doesn't have a focus trap.

Can we do an offline sync about those?

@flash1293
Copy link
Author

@chandlerprall Fixed the problems discussed offline

@chandlerprall
Copy link
Owner

Additional changes look good, replied to the setState/noop thread.

@snide
Copy link

snide commented Jan 20, 2021

@flash1293 Here's a branch of a branch of a branch with some small fixes to this PR.

flash1293#1

@flash1293
Copy link
Author

@chandlerprall Cleaned up remaining stuff, I think it's worth another look

Copy link
Owner

@chandlerprall chandlerprall left a comment

Choose a reason for hiding this comment

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

thanks again @flash1293 !!

@chandlerprall chandlerprall merged commit 80915fd into chandlerprall:feature/virtualized-datagrid Jan 26, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants