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

[EuiSuperDatePicker] Convert date popover styles to Emotion #7908

Merged
merged 9 commits into from
Jul 25, 2024

Conversation

cee-chen
Copy link
Member

@cee-chen cee-chen commented Jul 23, 2024

Summary

Part 2 of 3 (see #7904, #7909)

This PR contains two conversions of class components to function components - I strongly recommend following by commit for easier review. The actual Sass conversions are fairly minimal, but getting EuiAbsoluteTab and EuiRelativeTab to a state where they can use hooks was the majority of the work.

QA

https://eui.elastic.co/pr_7908/#/templates/super-date-picker should look the same as production with the following permutations:

  • Click the form control pretty date
  • Click either of the two dates to trigger the date popover
  • Absolute tab
    • Padding should look the same as production
    • Type invalid text (e.g. asdf) into the input. Confirm the formatting help text shows up and correctly overlaps into the submit button (should take up the full width of the popover)
    • Press enter or click the submit button and confirm the help text goes red to indicate an error message
    • Select all text in the input and paste the following unix timestamp: 1698793930. Confirm that the content automatically parses/validates on paste
  • Relative tab
    • Padding should look the same as production
    • Confirm the round switch updates the date timestamp as expected
    • Confirm the two selects update the date timestamp and parent super date picker control as expected
  • Now tab
    • Padding should look the same as production
  • Confirm the popover content (on all tabs) is the same width as production in desktop mode
  • Confirm the popover content sizes down to smaller width in mobile mode (~500px wide)

General checklist

  • Browser QA
    • Checked in mobile
    • Checked in Chrome, Safari, Edge, and Firefox
      - [ ] Checked in both light and dark modes - Size CSS only, no colors
      - [ ] Checked for accessibility including keyboard-only and screenreader modes
  • Docs site QA - N/A
  • Code quality checklist - N/A, no tests to update
  • Release checklist
    • A changelog entry exists and is marked appropriately.
      - [ ] If applicable, added the breaking change issue label (and filled out the breaking change checklist)
  • Designer checklist - N/A

- which will allow us to use a padding style hook in the future

+ remove unused `position` prop

+ reorder imports
- will allow us to use hooks for Emotion styles, might as well do this since we did EuiRelativeTab as well

- create new `isReadyToParse` state to replace old `isParsing` ref

+ remove unused `position` prop
@cee-chen cee-chen marked this pull request as ready for review July 24, 2024 00:32
@cee-chen cee-chen requested a review from a team as a code owner July 24, 2024 00:32
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.

Awesome work to convert more class components to function components! 🎉

🗒️ When checking the component I noticed a couple of regressions:

  1. On paste the absolute tab input adds the value twice. This then also results in a pasted timestamp not being parsed as expected (experienced in Chrome, Edge and Safari)

Screenshot 2024-07-24 at 17 14 27

  1. The focus styling on the date inputs changed: On previous production each input has a dedicated focus styling while now they have a shared one. Not sure this is expected? (I'm also not sure this is related to this PRs updates or the one before 🤔)
Screen.Recording.2024-07-24.at.17.22.24.mov

It worked in Firefox but not in webkit, derp
@cee-chen
Copy link
Member Author

cee-chen commented Jul 24, 2024

🤦 Thanks so much for catching the paste bug! It was working in FF bizarrely enough but not in webkit browsers. That should be fixed now in 8a2ab6b.

Regarding the updating highlight, that was broken in the previous PR. I'm also noticing a line wrapping issue I'll want to fix. If no objections, I'll create a separate PR for the fix!

Edit: PR with fix: #7911

@elasticmachine
Copy link
Collaborator

💚 Build Succeeded

History

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.

🚢 🐈‍⬛ Thanks for the fixes, it works as expected now! 🎉

@cee-chen
Copy link
Member Author

Thanks a million as always for your amazing QA Lene! ❤️‍🔥

@cee-chen cee-chen merged commit 3fec66b into elastic:main Jul 25, 2024
5 checks passed
@cee-chen cee-chen deleted the emotion/super-date-picker-2 branch July 25, 2024 16:04
jbudz pushed a commit to elastic/kibana that referenced this pull request Aug 1, 2024
`v95.4.0` ⏩ `v95.5.0`

_[Questions? Please see our Kibana upgrade
FAQ.](https://github.com/elastic/eui/blob/main/wiki/eui-team-processes/upgrading-kibana.md#faq-for-kibana-teams)_

---

## [`v95.5.0`](https://github.com/elastic/eui/releases/v95.5.0)

- Added `minusInSquare` and `plusInSquare` glyphs to `EuiIcon`.
([#7875](elastic/eui#7875))

**Bug fixes**

- Fixed `EuiSuperDatePicker` not correctly passing `refreshMinInterval`
from the quick select popover
([#7905](elastic/eui#7905))

**CSS-in-JS conversions**

- Converted `EuiSuperDatePicker`'s form control to Emotion;
([#7904](elastic/eui#7904))
  - Removed `$euiSuperDatePickerWidth`
  - Removed `$euiSuperDatePickerButtonWidth`
  - Removed `$euiSuperDatePickerNeedsUpdatingBackgroundColor`
  - Removed `$euiSuperDatePickerNeedsUpdatingTextColor`
  - Removed `@euiSuperDatePickerText` mixin
- Converted `EuiSuperDatePicker`'s date popover content to Emotion
([#7908](elastic/eui#7908))
- Converted `EuiSuperDatePicker`'s quick select to Emotion
([#7909](elastic/eui#7909))

---------

Co-authored-by: Elastic Machine <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants