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

[High Contrast Mode] Tables and data grid #8226

Merged
merged 8 commits into from
Dec 12, 2024

Conversation

cee-chen
Copy link
Contributor

@cee-chen cee-chen commented Dec 12, 2024

Summary

Note

This PR merges into a feature branch.

Saving my least favorite for last, as is tradition 😂

PR contains many VRT updates, but they've been split out into their own commits to make checking for visual diffs easier for singular changes.

QA

QA should generally consist of reviewing docs/stories in both light/dark modes and Windows/Mac high contrast modes and verifying that the updated components generally look non-broken/non-buggy in both.

General checklist

  • Browser QA - N/A
    • Checked in both light and dark modes
    • Checked in both MacOS and Windows high contrast modes
    • Checked in mobile
      - [ ] Checked in Chrome, Safari, Edge, and Firefox
      - [ ] Checked for accessibility including keyboard-only and screenreader modes
  • Docs site QA - N/A
  • Code quality checklist
    - [ ] Added or updated jest and cypress tests
  • Release checklist - N/A, feature branch
  • Designer checklist - N/A

@cee-chen cee-chen marked this pull request as ready for review December 12, 2024 08:09
@cee-chen cee-chen requested a review from a team as a code owner December 12, 2024 08:09
@kibanamachine
Copy link

Preview staging links for this PR:

@elasticmachine
Copy link
Collaborator

💚 Build Succeeded

@acstll
Copy link
Contributor

acstll commented Dec 12, 2024

I couldn't find anything buggy or broken, looks good (to my new-hire eyes)!

Just to double check, there's also fixes not strictly related to HCM like the spacing in the expanded row (screenshot below).

Screenshot 2024-12-12 at 14 22 50

TIL VRT refers to visual regression testing 😛
Also first time I see clip-path used like that, nice! 👌

Copy link
Contributor

@mgadewoll mgadewoll left a comment

Choose a reason for hiding this comment

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

🚢 🐈‍⬛ The changes look and work great 🎉
I just left two non-blocking comments.

Copy link
Contributor

Choose a reason for hiding this comment

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

Non blocking, likely unrelated (just attaching it here):
I noticed that the mobile actions seem to be placed differently between color modes.
It seems like the inset-block-start doesn't always apply. Maybe we could change to margin-block-start? 🤔

storybook

Screen.Recording.2024-12-12.at.14.45.00.mov

EUI docs

Screen.Recording.2024-12-12.at.14.52.51.mov

Copy link
Contributor Author

Choose a reason for hiding this comment

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

haha what the heck

Copy link
Contributor Author

@cee-chen cee-chen Dec 12, 2024

Choose a reason for hiding this comment

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

I think this is a bizarre Storybook/rendering thing. If I start the story in light mode, the opposite effect happens in dark mode. And if you refresh the page, it fixes itself again. I'm going to leave it alone for now to not break production styles 🤷 (couldn't get top: 0/margin-top to work either)

Copy link
Contributor

Choose a reason for hiding this comment

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

Huh, weird indeed. I was able to reproduce it in EUI docs as well. 🫤
I'm ok not touching it here, as it's not related to the changes done here. 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oooh I missed that in the second screencap, apologies. I can repro it in production EUI docs as well. With that in mind (since it's not caused by high contrast mode), can we file a separate issue for this bug? It's honestly bonkers haha, I don't think I've ever seen anything like it. I wonder if it's something to do with tables being tables 🫠

Copy link
Contributor

Choose a reason for hiding this comment

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

🔗 I added an issue for it.

@cee-chen
Copy link
Contributor Author

Just to double check, there's also fixes not strictly related to HCM like the spacing in the expanded row (screenshot below).

Yes, this was noted as a separate change/cleanup in my git commit history: edf83a2

Normally I write a bit longer commit messages explaining my rationale + add a PR description line informing devs to review by commit, but in this case I was very tired last night and also spoiled by Lene being used to my process 😅

I also didn't bother adding a changelog to it as it doesn't affect consumer usage or end-user functionality (it just looks a bit nicer).

@cee-chen cee-chen merged commit b7d545b into elastic:high-contrast-mode Dec 12, 2024
5 checks passed
@cee-chen cee-chen deleted the high-contrast-mode-10 branch December 12, 2024 17:22
@cee-chen cee-chen mentioned this pull request Dec 12, 2024
10 tasks
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.

5 participants