Skip to content

Conversation

@kc13greiner
Copy link
Contributor

@kc13greiner kc13greiner commented Jun 28, 2022

Summary

Removed the Flex styled elements around the Form Label as suggested here

I didn't see a 1px jump, so I didn't include vertical-align: text-top; styling on the icon, but please let me know if you still see the jump and we can discuss.

UPDATE: After syncing with Oleg, Im now able to see the 1px jump. vertical-align: text-top; doesn't appear to fix the issue (locally), @MichaelMarcialis Do you have any suggestions on what we could try next?

@kc13greiner kc13greiner added Team:Security Platform Security: Auth, Users, Roles, Spaces, Audit Logging, etc t// release_note:skip Skip the PR/issue when compiling release notes ci:deploy-cloud v8.4.0 labels Jun 28, 2022
@kc13greiner kc13greiner marked this pull request as ready for review June 28, 2022 14:47
@kc13greiner kc13greiner requested a review from a team as a code owner June 28, 2022 14:47
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-security (Team:Security)

Comment on lines 49 to 51
<>
{props.children} {!isEqual ? <EuiIcon type="dot" color="success" /> : undefined}
</>
Copy link
Member

Choose a reason for hiding this comment

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

issue: Just to make sure I wasn't going crazy I decided to leverage tools to see if the "jump" I observe (video) when I switch avatar modes is real 🤣 I see this in both Firefox and Chrome with this new markup. Do you see this too or is it just issue on my machine?

Screenshot from 2022-06-29 11-46-34

Screenshot from 2022-06-29 11-48-02

Copy link
Contributor

@MichaelMarcialis MichaelMarcialis 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 making these changes @kc13greiner! Left one suggestion below as an alternative to the previous vertical alignment suggestion. I think that should fix it for all browsers. Assuming it does, going to go ahead and approve this now so I don't hold you up.

) : undefined}
</EuiFlexGroup>
<>
{props.children} {!isEqual ? <EuiIcon type="dot" color="success" /> : undefined}
Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, looks like the vertical alignment styling I suggested was fixing it in Chrome, but not Firefox. In that case, let's decrease the icon size to prevent this from happening by using the size prop on EuiIcon. I think it ends up looking better and less distracting at this smaller size anyway, and it seems to correct the issue in all browsers.

Suggested change
{props.children} {!isEqual ? <EuiIcon type="dot" color="success" /> : undefined}
{props.children} {!isEqual ? <EuiIcon type="dot" color="success" size="s" /> : undefined}

Copy link
Contributor

@MichaelMarcialis MichaelMarcialis 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 making these changes. LGTM!

Copy link
Member

@azasypkin azasypkin left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@kibana-ci
Copy link

💚 Build Succeeded

Metrics [docs]

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id before after diff
security 504.6KB 504.5KB -85.0B

History

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

@kc13greiner kc13greiner merged commit 73e39a2 into elastic:main Jul 5, 2022
@kibanamachine kibanamachine added the backport:skip This PR does not require backporting label Jul 5, 2022
@tylersmalley tylersmalley added ci:cloud-deploy Create or update a Cloud deployment and removed ci:deploy-cloud labels Aug 17, 2022
@kc13greiner kc13greiner deleted the feature/simplify_labels branch January 26, 2024 20:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

backport:skip This PR does not require backporting ci:cloud-deploy Create or update a Cloud deployment release_note:skip Skip the PR/issue when compiling release notes Team:Security Platform Security: Auth, Users, Roles, Spaces, Audit Logging, etc t// v8.4.0

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants