elastic / eui

Elastic UI Framework 🙌
https://eui.elastic.co/
Other
6.09k stars 835 forks source link

[EuiContextMenuItem] Text bottom is cut off when `href` prop is supplied #3092

Closed justinkambic closed 2 years ago

justinkambic commented 4 years ago

I have noticed that the bottom of an EuiContextMenuItem is cut off when I specify the href prop for its content. Screenshot (look at the letter g in the text that says "Manage alerts"):

image

The implementation of the item looks like:

          <EuiContextMenuItem
            data-test-subj="xpack.uptime.navigateToAlertingUi"
            icon="gear"
            key="navigate-to-alerting"
            href={kibana.services?.application?.getUrlForApp(
              'kibana#/management/kibana/triggersActions/alerts'
            )}
          >
            <FormattedMessage
              id="xpack.uptime.navigateToAlertingButton.content"
              defaultMessage="Manage alerts"
            />
          </EuiContextMenuItem>,

If I remove the href prop, I no longer see the issue: image

This is happening for me in Firefox Dev Edition 75.0b2.

cchaos commented 4 years ago

I looked deeper into this and the problem stems from the combination of line-height and padding.

Rendered as links

Screen Shot 2020-03-16 at 09 56 00 AM

The overall height is correct, but because of the short line-height, the descenders get cutoff.

Rendered as buttons

Screen Shot 2020-03-16 at 09 55 54 AM

Somehow the line-height is larger, increasing the overall height by 4px incorrectly as it should match the same height as links.


Solutions

1. Fix it properly but scarily The correct fix is to actually set the line-height of plain buttons in the reset to line-height: inherit which will get a value of 1 from body.

Screen Shot 2020-03-16 at 09 55 16 AM

2. Fix it just for context menu Add the same line-height: inherit or 1 to .euiContextMenuItem to ensure both buttons and links render the same line-height.


In either case there would still need to be adjustments to fix the actual problem in the case of the line-height/padding combo.

malviyanshiv commented 4 years ago

@cchaos , I further explored the issue and found that the root of the issue is line-height property of <span class="euiContextMenu__itemLayout">, resulting in a height difference of 4px. For better explaination, The HTML markup generated in both the cases ( with and without href argument ) is as follows : with href prop withhyperlink

without href prop withouthyperlink

The <span class="euiContextMenu__itemLayout"> in the first case is enclosed with anchor tag, hence it inherits is line-height from body with value of 1 (giving it a height of 16px). But in the later case since <span class="euiContextMenu__itemLayout"> is enclosed in button, hence it inherits its line-height from button with value of normal (giving it a height of 21.818px).

malviyanshiv commented 4 years ago

@cchaos , Can I fix this issue by adding line-height: normal; to <span class="euiContextMenu__itemLayout"> ?

cchaos commented 4 years ago

Thanks @malviyanshiv , for the extra details. Yes, I believe your fix would fix the inconsistent sizing between button and anchor tags. However, it wouldn't fully solve the original issue which is that the descenders of the letters get cut off because the line-height is too small. There would also need to be tweaks to increase the line-height for both, and decrease the vertical padding. If you think you have a good solution for this, feel free to put up a PR.

malviyanshiv commented 4 years ago

@cchaos Sure, I will raise the PR.

ryankeairns commented 3 years ago

Just ran into this one on the profile menu PR:

Screen Shot 2020-11-04 at 4 59 53 PM
cchaos commented 2 years ago

It doesn't look like this is a problem anymore with the smaller font-size (14px) introduces in Amsterdam

image image