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 Solutions][Detection Engine] Updates the edit rules page to only have what is selected for editing #79233

Merged

Conversation

FrankHassanabad
Copy link
Contributor

@FrankHassanabad FrankHassanabad commented Oct 1, 2020

Summary

Before when you would edit rules you get all the rules as disabled but you cannot switch between them in edit mode as it's already a rule you created:
Screen Shot 2020-10-01 at 5 06 18 PM

After, now we remove those cards and only show the card of the rule type you're editing:
Screen Shot 2020-10-02 at 6 29 48 PM

Changes the card's icon placement and text.

Before:
Screen Shot 2020-10-02 at 9 27 44 AM

After:
Screen Shot 2020-10-02 at 6 31 01 PM

Fixes the Schedule/Actions and weirdness with CSS.
Before:
Screen Shot 2020-10-02 at 5 42 09 PM
Screen Shot 2020-10-02 at 5 42 05 PM

After:
Screen Shot 2020-10-02 at 5 42 51 PM

Screen Shot 2020-10-02 at 5 42 57 PM

Checklist

Delete any items that are not applicable to this PR.

For maintainers

@elasticmachine
Copy link
Contributor

Pinging @elastic/siem (Team:SIEM)

@FrankHassanabad FrankHassanabad added the Feature:Detection Rules Security Solution rules and Detection Engine label Oct 1, 2020
@spong spong added the release_note:skip Skip the PR/issue when compiling release notes label Oct 1, 2020
@@ -47,54 +47,54 @@ export const SelectRuleType: React.FC<SelectRuleTypeProps> = ({
const setQuery = useCallback(() => setType('query'), [setType]);
const setThreshold = useCallback(() => setType('threshold'), [setType]);
const setThreatMatch = useCallback(() => setType('threat_match'), [setType]);
const mlCardDisabled = isReadOnly || !hasValidLicense || !isMlAdmin;
const licensingUrl = useKibana().services.application.getUrlForApp('kibana', {
path: '#/management/stack/license_management',
});

const eqlSelectableConfig = useMemo(
Copy link
Contributor

Choose a reason for hiding this comment

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

@FrankHassanabad it looks like these should've been explicitly typed as EuiCardSelectProps:

Suggested change
const eqlSelectableConfig = useMemo(
const eqlSelectableConfig: EuiCardSelectProps = useMemo(

While this doesn't behave exactly as I would expect (you can still pass it to the component), you at least get a type error when trying to reference the isVisible field, which does not exist on that type. This is why you're getting those react warnings in your tests (and likely in the browser as well).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's not technically that all the properties are being used within the EuiCard. For example the isVisible is only being used as a boolean to determine if the card should show up or not:

{querySelectableConfig.isVisible && (
          <EuiFlexItem>
            <EuiCard
              data-test-subj="customRuleType"
              title={i18n.QUERY_TYPE_TITLE}
              titleSize="xs"
              description={i18n.QUERY_TYPE_DESCRIPTION}
              icon={<EuiIcon size="xl" type="search" />}
              selectable={querySelectableConfig}
              layout="horizontal"
              textAlign="left"
            />
          </EuiFlexItem>
        )}

The errors I am getting look to be from within isVisible being set by something else under the covers within Eui and not this isVisible I use here (unless I'm missing something ... which I could be :-)).

.join(', '),
<WrapperPage>
<EuiFlexGroup direction="row" justifyContent="spaceAround">
<MaxWidthEuiFlexItem>
Copy link
Contributor Author

@FrankHassanabad FrankHassanabad Oct 3, 2020

Choose a reason for hiding this comment

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

NOTE: I did not change a bunch of code here.

Instead I just changed:

<WrapperPage restrictWidth>
       ....

to be:

<WrapperPage>
        <EuiFlexGroup direction="row" justifyContent="spaceAround">
          <MaxWidthEuiFlexItem>
                ....

@@ -273,151 +285,155 @@ const CreateRulePageComponent: React.FC = () => {

return (
<>
<WrapperPage restrictWidth>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

NOTE: I did not change a bunch of code here.

Instead I just changed:

<WrapperPage restrictWidth>
       ....

to be:

<WrapperPage>
        <EuiFlexGroup direction="row" justifyContent="spaceAround">
          <MaxWidthEuiFlexItem>
                ....

export const MaxWidthEuiFlexItem = styled(EuiFlexItem)`
max-width: 1000px;
overflow: hidden;
`;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

NOTE: Using this instead of the previous way as the previous way no longer works whenever the top level:

<Main ...

React component over the last few days has morphed into going from "display: block" to being "display: flex" and it is using a column based layout rather than previous row based. So now, we utilize this nifty MaxWidthEuiFlexItem here with EuiFlex in order to control our inner component and flip it back to being a row based element. This also removes all the other hand spun CSS that was centering things for the simpler Eui space around and text justifications. So much much better so far 👍

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

async chunks size

id before after diff
securitySolution 10.4MB 10.4MB -2.5KB

History

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

@FrankHassanabad FrankHassanabad merged commit 6da7dc8 into elastic:master Oct 3, 2020
@FrankHassanabad FrankHassanabad deleted the remove-cards-from-edit-screen branch October 3, 2020 15:08
FrankHassanabad added a commit that referenced this pull request Oct 3, 2020
… only have what is selected for editing (#79233) (#79386)

## Summary

Before when you would edit rules you get all the rules as disabled but you cannot switch between them in edit mode as it's already a rule you created:
<img width="1063" alt="Screen Shot 2020-10-01 at 5 06 18 PM" src="https://user-images.githubusercontent.com/1151048/94872518-0bdaba00-040a-11eb-8b7d-3b3a59980e99.png">

After, now we remove those cards and only show the card of the rule type you're editing:
<img width="1074" alt="Screen Shot 2020-10-02 at 6 29 48 PM" src="https://user-images.githubusercontent.com/1151048/94978954-50835580-04dd-11eb-9e08-8e473fc216bf.png">

Changes the card's icon placement and text.

Before:
<img width="1098" alt="Screen Shot 2020-10-02 at 9 27 44 AM" src="https://user-images.githubusercontent.com/1151048/94979008-9dffc280-04dd-11eb-8bce-88cd49b063a8.png">

After:
<img width="1190" alt="Screen Shot 2020-10-02 at 6 31 01 PM" src="https://user-images.githubusercontent.com/1151048/94979005-95a78780-04dd-11eb-92ec-e81bc3ce436e.png">

Fixes the Schedule/Actions and weirdness with CSS.
Before:
<img width="588" alt="Screen Shot 2020-10-02 at 5 42 09 PM" src="https://user-images.githubusercontent.com/1151048/94979030-c5568f80-04dd-11eb-9c91-683d75dc37da.png">
<img width="516" alt="Screen Shot 2020-10-02 at 5 42 05 PM" src="https://user-images.githubusercontent.com/1151048/94979035-c8ea1680-04dd-11eb-9049-120c9d676b1a.png">

After:
<img width="1099" alt="Screen Shot 2020-10-02 at 5 42 51 PM" src="https://user-images.githubusercontent.com/1151048/94979038-d0112480-04dd-11eb-8a2a-8eb314023669.png">

<img width="1067" alt="Screen Shot 2020-10-02 at 5 42 57 PM" src="https://user-images.githubusercontent.com/1151048/94979040-d30c1500-04dd-11eb-85ab-a09e4def6adb.png">

### Checklist

Delete any items that are not applicable to this PR.

- [x] [Unit or functional tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html) were updated or added to match the most common scenarios
- [ ] This was checked for [keyboard-only and screenreader accessibility](https://developer.mozilla.org/en-US/docs/Learn/Tools_and_testing/Cross_browser_testing/Accessibility#Accessibility_testing_checklist)
- [ ] This renders correctly on smaller devices using a responsive layout. (You can test this [in your browser](https://www.browserstack.com/guide/responsive-testing-on-local-server)
- [ ] This was checked for [cross-browser compatibility](https://www.elastic.co/support/matrix#matrix_browsers)

### For maintainers

- [ ] This was checked for breaking API changes and was [labeled appropriately](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process)

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
gmmorris added a commit to gmmorris/kibana that referenced this pull request Oct 5, 2020
* master: (128 commits)
  add core-js production dependency (elastic#79395)
  Add support for sharing saved objects to all spaces (elastic#76132)
  [Alerting UI] Display a banner to users when some alerts have failures, added alert statuses column and filters (elastic#79038)
  load js-yaml lazily (elastic#79092)
  skip flaky suite (elastic#77278)
  Fix agentPolicyUpdateEventHandler() to use app context soClient for creation of actions (elastic#79341)
  [Security Solution] Untitled Timeline created when first action is to add note (elastic#78988)
  [Security Solutions][Detection Engine] Updates the edit rules page to:wq! only have what is selected for editing (elastic#79233)
  Cleanup yarn.lock from duplicates (elastic#66617)
  [kbn/optimizer] implement more efficient auto transpilation for node (elastic#79052)
  [Ingest Manager] Rename Fleet setup and requirement, Fleet => Central… (elastic#79291)
  [core/server/plugins] don't run discovery in dev server parent process (take 2) (elastic#79358)
  [babel/register] remove from build (take 2) (elastic#79379)
  [Security Solution] Changes rules table tag display (elastic#77102)
  define integrationTestRoot in config file and use to define screensho… (elastic#79247)
  Revert "[babel/register] remove from build (elastic#79176)"
  skip flaky suite (elastic#75241)
  [Uptime] Synthetics UI (elastic#77960)
  [Security Solution] [Detections] Only display actions options if user has "read" privileges (elastic#78812)
  [babel/register] remove from build (elastic#79176)
  ...
gmmorris added a commit to gmmorris/kibana that referenced this pull request Oct 5, 2020
* master: (288 commits)
  add core-js production dependency (elastic#79395)
  Add support for sharing saved objects to all spaces (elastic#76132)
  [Alerting UI] Display a banner to users when some alerts have failures, added alert statuses column and filters (elastic#79038)
  load js-yaml lazily (elastic#79092)
  skip flaky suite (elastic#77278)
  Fix agentPolicyUpdateEventHandler() to use app context soClient for creation of actions (elastic#79341)
  [Security Solution] Untitled Timeline created when first action is to add note (elastic#78988)
  [Security Solutions][Detection Engine] Updates the edit rules page to only have what is selected for editing (elastic#79233)
  Cleanup yarn.lock from duplicates (elastic#66617)
  [kbn/optimizer] implement more efficient auto transpilation for node (elastic#79052)
  [Ingest Manager] Rename Fleet setup and requirement, Fleet => Central… (elastic#79291)
  [core/server/plugins] don't run discovery in dev server parent process (take 2) (elastic#79358)
  [babel/register] remove from build (take 2) (elastic#79379)
  [Security Solution] Changes rules table tag display (elastic#77102)
  define integrationTestRoot in config file and use to define screensho… (elastic#79247)
  Revert "[babel/register] remove from build (elastic#79176)"
  skip flaky suite (elastic#75241)
  [Uptime] Synthetics UI (elastic#77960)
  [Security Solution] [Detections] Only display actions options if user has "read" privileges (elastic#78812)
  [babel/register] remove from build (elastic#79176)
  ...
@MindyRS MindyRS added the Team: SecuritySolution Security Solutions Team working on SIEM, Endpoint, Timeline, Resolver, etc. label Sep 23, 2021
@elasticmachine
Copy link
Contributor

Pinging @elastic/security-solution (Team: SecuritySolution)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature:Detection Rules Security Solution rules and Detection Engine release_note:skip Skip the PR/issue when compiling release notes Team: SecuritySolution Security Solutions Team working on SIEM, Endpoint, Timeline, Resolver, etc. Team:SIEM v7.10.0 v7.11.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants