Skip to content

[EuiDataGrid] Refactor & write unit tests for fullscreen control + Standardize 'fullscreen' copy#5680

Merged
cee-chen merged 8 commits intoelastic:mainfrom
cee-chen:datagrid/fullscreen
Mar 9, 2022
Merged

[EuiDataGrid] Refactor & write unit tests for fullscreen control + Standardize 'fullscreen' copy#5680
cee-chen merged 8 commits intoelastic:mainfrom
cee-chen:datagrid/fullscreen

Conversation

@cee-chen
Copy link
Contributor

@cee-chen cee-chen commented Mar 2, 2022

Summary

This is essentially a 2 part PR, both parts centered around the fullscreen behavior of EuiDataGrid:

  1. [Tech debt] Refactors all disparate parts of our fullscreen logic (some previously lived in the main data_grid file, others in data_grid_toolbar) to a single selector hook/file, and writes missing unit tests for said fullscreen mode logic.

  2. [Copy] closes [EuiDataGrid] Typo in fullscreen tooltip #5653 and addresses all other components to convert full(-)screen -> fullscreen per Caroline's comment

⚠️ I recommend following along by commit because the copy changes in particular touch a huge swath of files.

Test coverage

Before:
before

After:
after

QA

Other than the 'fullscreen' tooltip text, no fullscreen functionality should have changed as a result of this PR:

  • Fullscreen mode button toggles correctly
  • Escape key to exit fullscreen mode works correctly
  • Ref setIsFullScreen demo should still work as before
  • restrictBody class should be correctly applied to and removed from the body

Checklist

- [ ] Check against all themes for compatibility in both light and dark modes
- [ ] Checked in mobile
- [ ] Checked in Chrome, Safari, Edge, and Firefox
- [ ] Checked for accessibility including keyboard-only and screenreader modes
- [ ] Props have proper autodocs and playground toggles
- [ ] Added documentation
- [ ] Checked Code Sandbox works for any docs examples

  • Added or updated jest and cypress tests
  • Checked for breaking changes and labeled appropriately
  • A changelog entry exists and is marked appropriately

cee-chen added 4 commits March 2, 2022 11:53
- follows other toolbar/control behavior
- makes it easier to unit test
- makes it easier to track different full-screen related logic/events in one place
+ add 'Enter' verb per Gail's suggestion

+ fix data grid test with misspelled 'screen'
@cee-chen cee-chen requested a review from chandlerprall March 2, 2022 20:32
@cee-chen
Copy link
Contributor Author

cee-chen commented Mar 2, 2022

@chandlerprall Does this warrant a changelog entry? The first part is definitely internal only, but I'm not sure we add entries for basic copy changes since in theory our i18n changelog should capture that 🤔

@kibanamachine
Copy link

Preview documentation changes for this PR: https://eui.elastic.co/pr_5680/

@chandlerprall
Copy link
Contributor

Does this warrant a changelog entry?

The i18n keys changed, which should be mentioned and even constitutes a breaking change because existing translations will need to be updated to match the new keys.

@cee-chen
Copy link
Contributor Author

cee-chen commented Mar 4, 2022

👍 Gotcha, I hadn't seen previous changelog entries with mention of copy changes before so I wasn't sure. I can definitely add a breaking changelog.

@cee-chen cee-chen added the breaking change PRs with breaking changes. (Don't delete - used for automation) label Mar 4, 2022
@kibanamachine
Copy link

Preview documentation changes for this PR: https://eui.elastic.co/pr_5680/

Copy link
Contributor

@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.

One more nit, otherwise changes LGTM. The new full_screen_selector & its test file should drop the underscore in full_screen to match it being one word everywhere else.

@cee-chen
Copy link
Contributor Author

cee-chen commented Mar 9, 2022

FWIW our many internal fullScreenX variables are also incorrectly cased in that point. But honestly I think it's easier to read 🙈 I'm not as fussed about internal-only var names though (vs English/human-readable sentences). No strong feelings either way so I'll change the filename at least!

@cee-chen
Copy link
Contributor Author

cee-chen commented Mar 9, 2022

Ohh that's right, the file name actually affects the i18n token name so this was a great call!

@cee-chen cee-chen enabled auto-merge (squash) March 9, 2022 17:22
@chandlerprall
Copy link
Contributor

Ohh that's right, the file name actually affects the i18n token name so this was a great call!

That side effect didn't occur to me, but even better! 😄

Copy link
Contributor

@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.

Re-approving for good luck

@kibanamachine
Copy link

Preview documentation changes for this PR: https://eui.elastic.co/pr_5680/

@cee-chen cee-chen merged commit 6cbdf4a into elastic:main Mar 9, 2022
@cee-chen cee-chen deleted the datagrid/fullscreen branch March 9, 2022 18:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

breaking change PRs with breaking changes. (Don't delete - used for automation)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[EuiDataGrid] Typo in fullscreen tooltip

3 participants