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

[Security Solution] Enhancement endpoint flyout UI fixes #117987

Merged
merged 13 commits into from
Nov 17, 2021

Conversation

academo
Copy link
Contributor

@academo academo commented Nov 9, 2021

Summary

This is a second attempt for #115758

Fixes:

  • Header font size
  • Summary rows font size
  • Align policy name link, revision and out of date badge in a single row.
  • Consistent columns spacing on different screen sizes.
  • Replace deprecated date-time API

Before
image

After
image

Checklist

Delete any items that are not applicable to this PR.

@academo academo added release_note:fix v8.0.0 Team:Defend Workflows “EDR Workflows” sub-team of Security Solution auto-backport Deprecated - use backport:version if exact versions are needed v8.1.0 labels Nov 9, 2021
@academo academo requested a review from a team as a code owner November 9, 2021 10:15
@elasticmachine
Copy link
Contributor

Pinging @elastic/security-onboarding-and-lifecycle-mgt (Team:Onboarding and Lifecycle Mgt)

@academo academo requested review from dasansol92 and removed request for joeypoon November 10, 2021 12:34
}
`;

const ColumnTitle = ({ children }: { children: React.ReactNode }) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggest you use memo()

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There's no need for memo here. In addition detailsResults (where this component is used) is already memoized.

description: (
<EuiText>
<EuiText size="xs">
{' '}
Copy link
Contributor

Choose a reason for hiding this comment

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

Not part of your changes, but just wondering if this was intentional or if perhaps it was just the IDE formatter that did it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is probably some previous PR mistake. I'll remove it.

<EndpointPolicyLink
policyId={details.Endpoint.policy.applied.id}
data-test-subj="policyDetailsValue"
style={{ paddingRight: '5px' }}
Copy link
Contributor

Choose a reason for hiding this comment

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

Consider using a static object for the style value or memoizing it in order to avoid unnecessary re-renders. Same below

Copy link
Contributor Author

@academo academo Nov 11, 2021

Choose a reason for hiding this comment

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

There's no need for memo here. this component is literally a link, the win on using memo is probably worse in performance than not using it. Furthermore the variable where this is stored detailsResults is already memoized

I'll use className in this one as David suggested.

color="subdued"
size="xs"
className={'eui-displayInlineBlock'}
style={{ whiteSpace: 'nowrap', paddingRight: '5px' }}
Copy link
Contributor

Choose a reason for hiding this comment

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

FYI: I think Eui has a class name that can be added to className= prop for no wrap

Copy link
Contributor

Choose a reason for hiding this comment

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

Also we should avoid using the style prop and use styled components instead

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was wondering what was the go-to approach for this. I'll go for classNane.

@paul-tavares yes I totally forgot about that, thanks!

@academo
Copy link
Contributor Author

academo commented Nov 11, 2021

@elasticmachine merge upstream

@academo academo force-pushed the enhancement/endpoint-flyout-ui-fixes branch from 70c4593 to a8a1af4 Compare November 11, 2021 13:01
@academo
Copy link
Contributor Author

academo commented Nov 11, 2021

@elasticmachine merge upstream

@academo
Copy link
Contributor Author

academo commented Nov 15, 2021

@elasticmachine merge upstream

@kibana-ci
Copy link
Collaborator

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
securitySolution 2797 2796 -1

Async chunks

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

id before after diff
securitySolution 4.5MB 4.5MB +43.0B

History

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

@elastic elastic deleted a comment from kibanamachine Nov 15, 2021
@academo academo merged commit 473d9d6 into elastic:main Nov 17, 2021
kibanamachine added a commit to kibanamachine/kibana that referenced this pull request Nov 17, 2021
)

* initial UI changes for font size and deprecated dependency

* remove euitext wrapper

* Remove unused dependency

* Remove import

* Add text break word

* inline policy, version and icon

* Allign icon in the out_of_date component

* Remove unncessary wrapper

* Replace style attributes with className

Co-authored-by: Kibana Machine <[email protected]>
@kibanamachine
Copy link
Contributor

💚 Backport successful

Status Branch Result
8.0

This backport PR will be merged automatically after passing CI.

kibanamachine added a commit that referenced this pull request Nov 17, 2021
…118856)

* initial UI changes for font size and deprecated dependency

* remove euitext wrapper

* Remove unused dependency

* Remove import

* Add text break word

* inline policy, version and icon

* Allign icon in the out_of_date component

* Remove unncessary wrapper

* Replace style attributes with className

Co-authored-by: Kibana Machine <[email protected]>

Co-authored-by: Esteban Beltran <[email protected]>
TinLe pushed a commit to TinLe/kibana that referenced this pull request Nov 20, 2021
)

* initial UI changes for font size and deprecated dependency

* remove euitext wrapper

* Remove unused dependency

* Remove import

* Add text break word

* inline policy, version and icon

* Allign icon in the out_of_date component

* Remove unncessary wrapper

* Replace style attributes with className

Co-authored-by: Kibana Machine <[email protected]>
dmlemeshko pushed a commit that referenced this pull request Nov 29, 2021
* initial UI changes for font size and deprecated dependency

* remove euitext wrapper

* Remove unused dependency

* Remove import

* Add text break word

* inline policy, version and icon

* Allign icon in the out_of_date component

* Remove unncessary wrapper

* Replace style attributes with className

Co-authored-by: Kibana Machine <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
auto-backport Deprecated - use backport:version if exact versions are needed release_note:fix Team:Defend Workflows “EDR Workflows” sub-team of Security Solution v8.0.0 v8.1.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants