-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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] Adds getMockTheme function #92840
Conversation
x-pack/plugins/security_solution/public/common/components/and_or_badge/index.test.tsx
Outdated
Show resolved
Hide resolved
x-pack/plugins/security_solution/public/common/mock/get_mock_theme.ts
Outdated
Show resolved
Hide resolved
@elasticmachine merge upstream |
merge conflict between base and head |
Closes elastic/security-team#866.
💚 Build Succeeded
Metrics [docs]Async chunks
Page load bundle
History
To update your PR or re-run it, just comment with: cc @ecezalp |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changes LGTM, thank you. I had a few questions and one nit, but nothing preventing this from being merged.
@@ -30,8 +30,9 @@ import { | |||
} from '../../../../../common/detection_engine/schemas/response/rules_schema.mocks'; | |||
import { useRuleAsync } from '../../../../detections/containers/detection_engine/rules/use_rule_async'; | |||
import { AlertData } from '../types'; | |||
import { getMockTheme } from '../../../lib/kibana/kibana_react.mock'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
super nit: I prefer to keep these relative imports roughly organized by distance
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you have a neat way of doing so by pressing a few keys? Would be super interested in that!!!
eui: { | ||
euiColorDanger: '#ece', | ||
euiColorLightestShade: '#ece', | ||
euiColorPrimary: '#ece', | ||
euiFontWeightSemiBold: 'bold', | ||
euiFontWeightSemiBold: 1, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why the change here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
euiFontWeightSemiBold is a number! It's probably 400 or so in the light / dark theme
eui: { euiSizeS: '10px', euiLineHeight: '20px', euiBreakpoints: { s: '10px' }, euiSize: '10px' }, | ||
}; | ||
const mockTheme = getMockTheme({ | ||
eui: { euiSizeS: '10px', euiLineHeight: 10, euiBreakpoints: { s: '10px' }, euiSize: '10px' }, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ditto on the change here; does euiLineHeight
simply need a value to prevent the test from blowing up?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes
* master: (107 commits) [Logs UI] Fix log stream data fetching (elastic#93201) [App Search] Added relevance tuning search preview (elastic#93054) [Canvas] Fix reports embeddables (elastic#93482) [ILM] Added new functional test in ILM for creating a new policy (elastic#92936) Remove direct dependency on statehood package (elastic#93592) [Maps] Track tile loading status (elastic#91585) [Discover][Doc] Improve main documentation (elastic#91854) [Upgrade Assistant] Disable UA and add prompt (elastic#92834) [Snapshot Restore] Remove cloud validation for slm policy (elastic#93609) [Maps] Support GeometryCollections in GeoJson upload (elastic#93507) [XY Charts] fix partial histogram endzones annotations (elastic#93091) [Core] Simplify context typings (elastic#93579) [Alerting] Improving health status check (elastic#93282) [Discover] Restore context documentation (elastic#90784) [core-docs] Edits core-intro section for the new docs system (elastic#93540) add missed codeowners (elastic#89714) fetch node labels via script execution (elastic#93225) [Security Solution] Adds getMockTheme function (elastic#92840) Sort dependencies in package.json correctly (elastic#93590) [Bug] missing timepicker:quickRanges migration (elastic#93409) ...
Friendly reminder: Looks like this PR hasn’t been backported yet. |
2 similar comments
Friendly reminder: Looks like this PR hasn’t been backported yet. |
Friendly reminder: Looks like this PR hasn’t been backported yet. |
Closes elastic/security-team#866.
# Conflicts:
# x-pack/plugins/security_solution/public/detections/components/rules/query_preview/index.test.tsx
Closes elastic/security-team#866.
Summary
Types the mock themes in tests as EuiTheme.
Replaces the ThemeProvider (from styled-components) with EuiThemeProvider in app.js.
Closes elastic/security-team#866.
Checklist