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

[EuiButtonDisplay] Fix missing text-align CSS #8057

Merged
merged 1 commit into from
Oct 8, 2024

Conversation

cee-chen
Copy link
Member

@cee-chen cee-chen commented Oct 2, 2024

Summary

Note

This doesn't fundamentally affect any actual visuals, because flex CSS was already handling centering button content for us. But it's nice not to have buggy CSS-in-JS in our codebase 😅

This was missed during EuiButton's original Emotion conversion. Just for funsies, I'm going to dive into what was causing the bug:

  • Our logical*Style utilities output objects of CSS key/values, e.g. { color: red }

  • Our logical*CSS utilities output strings of CSS, e.g. 'color: red;'

  • Emotion CSS that uses the css`` template literal can parse both.

  • Regular template literals (``), like the one being used in euiButtonBaseCSS (to avoid generating an extra emotion label, presumably), cannot parse objects, only strings. So the previous code was generating this:

    Huge thank you/shout out to @tsullivan for logging and noticing this bug!

  • Changing our internal EUI code to use the string output instead of object output solves the issue. We now have text-align: center in our CSS again, where it was previously missing.

  • Bonus: another approach I could have tried to resolve this was potentially using the css`` template literal and renaming the util to _euiButtonBaseCSS. Emotion generally seems to respect and not add extra classNames to underscore-prefixed utilities, although this is not always the case.

QA

General checklist

  • Browser QA
    • Checked in Chrome, Safari, Edge, and Firefox
  • Docs site QA - N/A
  • Code quality checklist - N/A, styles only
  • Release checklist
    • [ ] A changelog entry exists and is marked appropriately. I'm opting to skip a changelog for this since it's an internal-only component / tech debt and doesn't affect any consumers or end-users.
  • Designer checklist - N/A

@cee-chen cee-chen marked this pull request as ready for review October 2, 2024 16:47
@cee-chen cee-chen requested a review from a team as a code owner October 2, 2024 16:47
@elasticmachine
Copy link
Collaborator

💚 Build Succeeded

@tkajtoch tkajtoch self-requested a review October 7, 2024 15:13
@cee-chen
Copy link
Member Author

cee-chen commented Oct 8, 2024

@elastic/eui-team Re-ping for any review :)

Copy link
Member

@tkajtoch tkajtoch left a comment

Choose a reason for hiding this comment

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

Tested on PR environment; the change looks great!

I kinda hate how easy would be to make that mistake again with these function names...

@cee-chen
Copy link
Member Author

cee-chen commented Oct 8, 2024

Very fair - we could try to add a lint rule for it 🤔 but most likely we just need strong visual regression testing?

@cee-chen cee-chen merged commit 96b19ff into elastic:main Oct 8, 2024
10 of 11 checks passed
@cee-chen cee-chen deleted the button/fix-style branch October 8, 2024 17:04
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.

3 participants