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

[EuiDataGrid] Redesigns feature branch #7382

Merged
merged 9 commits into from
Nov 23, 2023
Merged

[EuiDataGrid] Redesigns feature branch #7382

merged 9 commits into from
Nov 23, 2023

Conversation

cee-chen
Copy link
Contributor

@cee-chen cee-chen commented Nov 21, 2023

Summary

This is a feature branch of 3 separate EuiDataGrid redesigns:

Each individual PR should have already been fairly thoroughly code reviewed (see each individual link for more screenshots, context, etc.). This feature branch PR is intended primarily for a final design review and QA pass of all combined features / the whole EuiDataGrid component.

QA

General checklist

  • Browser QA
    • Checked in both light and dark modes
    • Checked in mobile
    • Checked in Chrome, Safari, Edge, and Firefox
      - [x] Checked for accessibility including keyboard-only and screenreader modes NOTE: There are some issues here, but they're not specific to the changes made, and already exist in production
  • Docs site QA - Evaluated per-PR
  • Code quality checklist - Evaluated per-PR
  • Release checklist - Evaluated per-PR
  • Designer checklist
    • Updated the Figma library counterpart

@cee-chen cee-chen marked this pull request as ready for review November 22, 2023 00:26
@cee-chen cee-chen requested a review from a team as a code owner November 22, 2023 00:26
@cee-chen
Copy link
Contributor Author

@MichaelMarcialis Do you mind giving EuiDataGrid a final design pass and approving if everything looks good to you?

@ryankeairns
Copy link
Contributor

Hey @cee-chen , nice work!
I ran through the QA url with a variety of screen and container sizes; keyboard and mouse; etc.

The only issue I have run across is this one where the styles appear broken when a cell is expanded:

CleanShot 2023-11-22 at 08 26 36@2x

- cell actions bg color was missing on popover open - always defaulting to color primary fixes the issue
@cee-chen

This comment was marked as resolved.

Copy link
Contributor

@ryankeairns ryankeairns left a comment

Choose a reason for hiding this comment

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

Thanks for the extra effort here to get this lined up for the Discover team!

I checked locally and see the fix for the one item I noted above.

CleanShot 2023-11-22 at 09 28 33@2x

@MichaelMarcialis
Copy link
Contributor

@MichaelMarcialis I noticed your design file has the actions dropped by two pixels. Is that still desired or perhaps this changed along the way?

@ryankeairns: I suggested this approach to avoid having it appear as shown in this quick mockup below:

CleanShot 2023-11-22 at 13 33 06

I'm not a fan of the weird indents the rounded corners create in this approach. I also don't like that the bottom edge of the action background isn't flush with the inner edge of the cell border (as it feels misaligned).

Copy link
Contributor

@MichaelMarcialis MichaelMarcialis left a comment

Choose a reason for hiding this comment

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

Thanks so much for putting this together, @cee-chen! These changes look and work wonderfully. I'm not seeing anything critical that needs to be addressed before merging, so I'm approving now to avoid blocking you. I only have a few comments below regarding minor things I noticed (which can be split off as separate issues if you prefer):

  • In the case of an excessively narrow data grid column, while hovering/focusing a cell with more than two cell actions, the actions container correctly renders wider than the cell it originates from. The only thing I'd like to see is the bottom-right corner of the actions container to have a 2px border radius (or bottom-left corner if right-aligned cell contents). This will more closely match the designs.

    CleanShot 2023-11-22 at 13 37 28

  • In the case that a cell doesn't have any cell-based actions attached, should we supress/remove the hover/focus inner cell borders? Since no actions will eminate from it, I'm not sure if we need it. But I can see an argument being made for consistency with other cells, so wanted to ask you and @ryankeairns.

    CleanShot 2023-11-22 at 13 40 19

  • For the data grid column heading popovers, would it be possible to have the popover alignment and arrow appear as if it is eminating from the boxesVertical icon that appears on hover/focus? I know it's a super minor nitpick, but I think that may end up looking nicer than situations like the screenshot below, where it appears to be eminating from whitespace.

    CleanShot 2023-11-22 at 13 41 04

@cee-chen
Copy link
Contributor Author

Those are great suggestions, thanks Michael! I'll take a run at at the 1st and 3rd bullet point here later today and post the screenshots.

In the case that a cell doesn't have any cell-based actions attached, should we supress/remove the hover/focus inner cell borders?

My 2c is I'd rather keep it for consistency.

- this prevents a visual bug when the popover is open *above* the cell instead of above

- it also fills the gap that occurs between the cell actions and the popover
- this is actually cleaner CSS anyway, win/win!
@cee-chen
Copy link
Contributor Author

Bullet point 1 addressed in 937a033:

Bullet point 3 addressed in e1c802c:

@cee-chen
Copy link
Contributor Author

Not sure if you're still around for the day @MichaelMarcialis - if not no worries, I'll go ahead and merge this at the end of my work day. If you notice anything else, please don't hesitate to mention it even post-merge, we can always open a new follow-up PR!

Also noting for posterity - I normally wouldn't rush this as much, but 8.12 FF and team travel is causing an artificial compression of timelines 🤞

@ryankeairns
Copy link
Contributor

ryankeairns commented Nov 22, 2023

Thank you!
Checked both: narrow column looks good; as for popover from header action button...

Last nit, should the euiDataGridHeaderCell__icon class have a justify-content: center or space-around on it?

Before

CleanShot 2023-11-22 at 14 54 10@2x

After

CleanShot 2023-11-22 at 14 58 09@2x

@cee-chen
Copy link
Contributor Author

Last nit, should the euiDataGridHeaderCell__icon class have a justify-content: center or space-around on it?

I'm not totally sure why it should? Do you mind explaining more? 🤔

@ryankeairns
Copy link
Contributor

It will result in the arrow/pointy part of the popover over centering with the header action button. This is nitpicky feedback, I admit. End result:

CleanShot 2023-11-22 at 15 06 17@2x

@cee-chen
Copy link
Contributor Author

It will result in the arrow/pointy part of the popover over centering with the header action button

Wait, am I missing something? Isn't that what Marcialis was asking for?

@cee-chen
Copy link
Contributor Author

Ryan yarned with me on this on Slack, and I was indeed missing something! Should be all fixed as of the last commit. We also had a good yarn about the controller UX for Read Dead Redemption 2, Horizon, and Baldur's Gate 3 😎

@kibanamachine
Copy link

Preview staging links for this PR:

@elasticmachine
Copy link
Collaborator

💚 Build Succeeded

History

@cee-chen cee-chen merged commit 88cb79c into main Nov 23, 2023
7 checks passed
@cee-chen cee-chen deleted the datagrid-redesigns branch November 23, 2023 00:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants