Skip to content

Less wrapper elements#18

Merged
chandlerprall merged 20 commits intochandlerprall:feature/virtualized-datagridfrom
flash1293:flash1293/less-wrappers
Jan 26, 2021
Merged

Less wrapper elements#18
chandlerprall merged 20 commits intochandlerprall:feature/virtualized-datagridfrom
flash1293:flash1293/less-wrappers

Conversation

@flash1293
Copy link
Copy Markdown

@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
Copy Markdown
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

@chandlerprall
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
Author

@chandlerprall Fixed the problems discussed offline

@chandlerprall
Copy link
Copy Markdown
Owner

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

@snide
Copy link
Copy Markdown

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
Copy Markdown
Author

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

Copy link
Copy Markdown
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