eui icon indicating copy to clipboard operation
eui copied to clipboard

[EuiTabs] Remove `xl` size

Open ryankeairns opened this issue 10 months ago • 12 comments

Description

The purpose of this task is to remove the xl size in EuiTabs.

It's been discussed in https://github.com/elastic/eui/discussions/8128. Two reasons for this request are:

  1. Subjectively, it is an atypical font size for tabs (see below). The guidelines say to only use for page titles only, but I don't believe this is a justifiable recommendation.
  2. Objectively, we don't use it in Kibana. Search for usage with this regex <EuiTabs[ \w=\{\}>\(\)\n;".-:]* size="xl".

[!NOTE] This is a breaking change with low impact. We can suggest using l as the recommended fix.

CleanShot 2024-11-10 at 21 51 08@2x

Acceptance criteria

  • [x] xl size is removed from EuiTabs

ryankeairns avatar Jan 31 '25 23:01 ryankeairns

Sorry @ryankeairns, we missed this one. It wasn't added to our team board automatically for some reason. It's here now, we'll take a look.

JasonStoltz avatar May 09 '25 13:05 JasonStoltz

The code changes are small. There are no usages in Kibana and Cloud UI (used the regexp from the description). We should treat it as a breaking change regardless (also to account for external contributors). I put my preliminary estimation.

weronikaolejniczak avatar May 29 '25 09:05 weronikaolejniczak

Thanks all.

Another place these can surface is through EuiPageTemplate (docs) when no heading is provided. In any case, my reasons stand :) ... and I will address feedback that comes as a result of the change.

ryankeairns avatar May 29 '25 15:05 ryankeairns

I assume we'll need to make sure this is updated in Figma after we make this change as well.

JasonStoltz avatar Jun 05 '25 13:06 JasonStoltz

Labeled this as a breaking change -- we should consider pairing this with other breaking changes in a planned release. If we do that, we should plan for a breaking change release and give folks extra time to test in Kibana.

JasonStoltz avatar Jun 05 '25 13:06 JasonStoltz

I assume we'll need to make sure this is updated in Figma after we make this change as well.

Created an issue on our EUI Design work board and will have this ready to roll in conjunction with the component change.

ryankeairns avatar Jun 05 '25 20:06 ryankeairns

@ryankeairns We have to account for direct EuiPageHeader and KibanaPageTemplate.Header usages (that use EuiPageHeader underneath). Specifically, those that pass tabs and NOT pageTitle at the same time. They fall into this condition:

https://github.com/elastic/eui/blob/803bdb959de54eff4aa96789a3644885370bd39d/packages/eui/src/components/page/page_header/page_header_content.tsx#L273-L326

In theory, there will be no code changes on Kibana side but there could be visual changes (and there could be snapshot changes). I have a PR open just for removing the size: https://github.com/elastic/eui/pull/8762

Do we want to proceed or do we want to pause the change?

weronikaolejniczak avatar Jun 06 '25 17:06 weronikaolejniczak

Thanks @weronikaolejniczak

This is helpful. With this information, I will communicate the proposed change to the EUI ambassadors (meeting next week) so that they are aware and have the opportunity to voice any concerns.

So, yes, hold for just a bit, and I will knock on some doors.

ryankeairns avatar Jun 06 '25 18:06 ryankeairns

@ryankeairns just before falling asleep I realized that I read the condition wrong 😅 The affected cases would be if there are tabs passed but NO pageTitle. I searched in Kibana and Cloud, and I didn't find any EuiPageHeader or KibanaPageTemplate.Header that pass tabs and not pageTitle (so they all should use the l size). That being said, might be worth to double-check by someone and raise this change on the EUI ambassadors meeting.

weronikaolejniczak avatar Jun 07 '25 12:06 weronikaolejniczak

@weronikaolejniczak Should we move this to blocked until Ryan has a chance to talk to the ambassadors?

JasonStoltz avatar Jun 10 '25 12:06 JasonStoltz

@JasonStoltz there are no usages of EuiPageHeader or KibanaPageTemplate.Header, so I think we're fine to proceed but I can put it in Blocked for now anyway.

weronikaolejniczak avatar Jun 10 '25 13:06 weronikaolejniczak

@ryankeairns just before falling asleep I realized that I read the condition wrong 😅 The affected cases would be if there are tabs passed but NO pageTitle. I searched in Kibana and Cloud, and I didn't find any EuiPageHeader or KibanaPageTemplate.Header that pass tabs and not pageTitle (so they all should use the l size). That being said, might be worth to double-check by someone and raise this change on the EUI ambassadors meeting.

Got it. I also could not find any use cases of this. I have sent a message in #ux-wg-patterns and will reiterate the point on Thursday, in person.

ryankeairns avatar Jun 10 '25 16:06 ryankeairns

Thanks for following through on this. I received 0 feedback and interpret that to mean there would be little to no impact.

ryankeairns avatar Jun 25 '25 17:06 ryankeairns