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

Removing Flex styled elements #135317

Merged
merged 6 commits into from
Jul 5, 2022

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 Team focused on: Auth, Users, Roles, Spaces, Audit Logging, and more! 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
Collaborator

💚 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 commit 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 commit 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 Team focused on: Auth, Users, Roles, Spaces, Audit Logging, and more! v8.4.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants