-
Notifications
You must be signed in to change notification settings - Fork 858
feat(website): add size="s" to all content EuiText wrappers #9078
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
feat(website): add size="s" to all content EuiText wrappers #9078
Conversation
💚 Build Succeeded |
💚 Build Succeeded
|
[not a direct feedback to this PR] I don't know how I feel about the fact that a prop to an EUI component needs wrapping with EuiText in order to render text content correctly. I strongly believe a design system component library should provide necessary defaults so that such wrappers are never needed.
I understand this sentence as "if you write naked / custom HTML you'll need to wrap text with EuiText for styling". Passing a simple paragraph to EuiTour is a bit of a stretch... LMK what you think! |
sharing my 2¢, this sounds like the never-ending struggle between consistency vs flexibility, providing sensible defaults is not always straightforward.
I agree with this. I would be in favor of having this PR merged and creating a new issue to update the API so passing |
@tkajtoch @acstll we had this conversation with @ek-so and @ryankeairns on the issue. I recommend reading it through :) Thank you for your input, I'd appreciate a review. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @weronikaolejniczak for bringing up the discussion conversation, I hadn't read it before you shared it 🫣. I think the discussion is not exactly the same: here it's mostly about the API in EuiTour
and more exactly the need for the EuiText
wrapper in order to have consistent font sizes. Would you agree?
Keeping in mind there's a need to revisit font sizes in general later, did you consider adding euiFontSize(_, 's')
to the .euiTour__content
node? That way we don't need EuiText
, and even passing just text to the content
prop will always yield 14px as expected.
<EuiTourStep
- content={
- <EuiText size="s">
- <p>The tour step content.</p>
- </EuiText>
- }
+ content="The tour step content."
@acstll totally agree with your proposal, and I appreciate you taking the time to dive into this! 💚 I actually raised a similar point in an earlier comment (#9037 (comment)):
Since that part of the thread didn't get picked up at the time, I took a more immediate, lower-impact route - updating the docs for consistency and usability based on what is the current component state. I also noted a broader fix (like the one you're outlining now) would need its own issue and discussion:
That said, I think your idea of adding Let me know what you'd prefer - I’m happy either way, just want to keep us moving forward 👍 |
Since the change I suggest would also mean updating the docs (in this case to remove any But I'm fine with either option. And as discussed elsewhere, I'm in favor of landing this quick fix now and working on any follow-up updates later on. I will do a quick check and approve. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🟢 Everything looking as expected (thanks also for the other few fixes in there!)
Thanks for the review 🎉 I removed the link to the issue, will unassign myself and put into the backlog. If you have the time in your support duty week, feel free to take it! |
Summary
Change the
EuiTour
docs examples to useEuiText
as acontent
wrapper and setsize="s"
.Why are we making this change?
We want the
content
inEuiTour
to use this composition:as outlined by our docs:
That composition yields undesirable
font-size
, specifically16px
. The hierarchy between the title and the content seems off:Therefore, we want all our docs examples to use
size="s"
:Impact to users
🟢 None. It's a website change.
QA
Specific checklist
14px
)