-
Notifications
You must be signed in to change notification settings - Fork 841
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
[EuiBeacon] Add color prop #6420
Conversation
Since this is a community submitted pull request, a Jenkins build has not been kicked off automatically. Can an Elastic organization member please verify the contents of this patch and then kick off a build manually? |
💚 CLA has been signed |
…with individual color keys instead
jenkins test this |
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.
This looks great to me from a code POV. @miukimiu, do you think this new prop needs any more documentation than it currently has (just props table) or is it good to go?
describe('size', () => { | ||
it('accepts size', () => { | ||
const component = render(<EuiBeacon size={14} {...requiredProps} />); | ||
|
||
expect(component).toMatchSnapshot(); | ||
}); | ||
}); |
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.
Very minor / optional code organization - I'd add a newline between blocks and leave this as its own test
vs giving it a describe block
describe('size', () => { | |
it('accepts size', () => { | |
const component = render(<EuiBeacon size={14} {...requiredProps} />); | |
expect(component).toMatchSnapshot(); | |
}); | |
}); | |
test('size', () => { | |
const component = render(<EuiBeacon size={14} {...requiredProps} />); | |
expect(component).toMatchSnapshot(); | |
}); |
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.
@constancecchen I agree. I tried to commit, but build process is failing with this error.
src/components/modal/modal.tsx:84:60 - error TS2769: No overload matches this call.
Overload 1 of 2, '(props: EuiFocusTrapProps | Readonly<EuiFocusTrapProps>): EuiFocusTrap', gave the following error.
Type '{ children: Element; initialFocus: string | HTMLElement | (() => HTMLElement) | undefined; scrollLock: true; preventScrollOnFocus: true; }' is not assignable to type 'IntrinsicAttributes & IntrinsicClassAttributes<EuiFocusTrap> & Pick<Readonly<EuiFocusTrapProps> & Readonly<...>, "className" | ... 17 more ... | "closeOnMouseup"> & Partial<...> & Partial<...>'.
Property 'preventScrollOnFocus' does not exist on type 'IntrinsicAttributes & IntrinsicClassAttributes<EuiFocusTrap> & Pick<Readonly<EuiFocusTrapProps> & Readonly<...>, "className" | ... 17 more ... | "closeOnMouseup"> & Partial<...> & Partial<...>'.
Overload 2 of 2, '(props: EuiFocusTrapProps, context: any): EuiFocusTrap', gave the following error.
Type '{ children: Element; initialFocus: string | HTMLElement | (() => HTMLElement) | undefined; scrollLock: true; preventScrollOnFocus: true; }' is not assignable to type 'IntrinsicAttributes & IntrinsicClassAttributes<EuiFocusTrap> & Pick<Readonly<EuiFocusTrapProps> & Readonly<...>, "className" | ... 17 more ... | "closeOnMouseup"> & Partial<...> & Partial<...>'.
Property 'preventScrollOnFocus' does not exist on type 'IntrinsicAttributes & IntrinsicClassAttributes<EuiFocusTrap> & Pick<Readonly<EuiFocusTrapProps> & Readonly<...>, "className" | ... 17 more ... | "closeOnMouseup"> & Partial<...> & Partial<...>'.
84 <EuiFocusTrap initialFocus={initialFocus} scrollLock preventScrollOnFocus>
~~~~~~~~~~~~~~~~~~~~
Found 1 error.
I checked EuiFocusTrap
's props and there's no preventScrollOnFocus
prop.
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.
That's a super bizarre error since it's not related to EuiBeacon 🤔 - I recently merged in latest main into this branch in order to kickstart CI, so I'm guessing what happened is you may need to re-run yarn
to fix type definitions.
No worries at all though as this was a super optional comment! Thank you again for your great work!
Preview documentation changes for this PR: https://eui.elastic.co/pr_6420/ |
I think for now it's ok to just have the props table. |
Going to go ahead and merge this. Thanks again @dawitam11! |
@miukimiu, do you mind handling the "Updated the Figma library counterpart" checklist step separately from this PR? |
`eui@70.2.4` ⏩ `eui@70.4.0` - "Fixed EuiButtonGroup firing onChange twice" required changing some tests from `click` to `change` ___ ## [`70.4.0`](https://github.com/elastic/eui/tree/v70.4.0) - Updated `EuiTourStep.footerAction` type to accept `ReactNode[]` ([#6384](elastic/eui#6384)) - Vertically aligned all footer content so that `euiTourStepIndicator` is always centered ([#6384](elastic/eui#6384)) - Added `filterInCircle` glyph to `EuiIcon` ([#6385](elastic/eui#6385)) - Added `color` prop to `EuiBeacon` ([#6420](elastic/eui#6420)) - Added the `euiMaxBreakpoint` and `euiMinBreakpoint` CSS-in-JS utilities for creating min/max-width media queries ([#6431](elastic/eui#6431)) **Bug fixes** - Restores the previous match operator behaviour when the query value is split into multiple terms after analysis. ([#6409](elastic/eui#6409)) - Fixed missing slide-in animation on `EuiCollapsibleNav`s and left-side `EuiFlyout`s ([#6422](elastic/eui#6422)) - Fix bug in `EuiCard` where footer were not aligned to the bottom of the card ([#6424](elastic/eui#6424)) - Fixed multiple component media queries for consumers with custom theme breakpoints ([#6431](elastic/eui#6431)) ## [`70.3.0`](https://github.com/elastic/eui/tree/v70.3.0) - `EuiSearchBar` now automatically wraps special characters not used by query syntax in quotes ([#6356](elastic/eui#6356)) - Added `alignment` prop to `EuiBetaBadge` ([#6361](elastic/eui#6361)) - `EuiButton` now accepts `minWidth={false}` ([#6373](elastic/eui#6373)) **Bug fixes** - Fixed `EuiPageTemplate` not correctly passing the `component` prop to the inner main content wrapper. ([#6352](elastic/eui#6352)) - `EuiSkipLink` now correctly calls `onClick` even when `fallbackDestination` is invalid ([#6355](elastic/eui#6355)) - Permanently fixed `EuiModal` to not cause scroll-jumping issues on modal open ([#6360](elastic/eui#6360)) - Re-fixed `EuiPageSection` not correctly merging `contentProps.css` ([#6365](elastic/eui#6365)) - Fixed `EuiTab` not defaulting to size `m` ([#6366](elastic/eui#6366)) - Fixed the shadow sizes of `.eui-yScrollWithShadows` and `.eui-xScrollWithShadows` ([#6374](elastic/eui#6374)) - Fixed bug in `EuiCard` where the inner content in vertical cards was not growing 100% in width ([#6377](elastic/eui#6377)) - Fixed incorrect margins in `EuiSuperDatePicker` caused by `EuiFlex` CSS gap change ([#6380](elastic/eui#6380)) - Fixed visual bug in nested `EuiFlexGroup`s, where the parent `EuiFlexGroup` is responsive but a child `EuiFlexGroup` is not ([#6381](elastic/eui#6381)) **CSS-in-JS conversions** - Converted `EuiModal` to Emotion ([#6321](elastic/eui#6321)) **Fixes** - `EuiButton` no longer outputs unnecessary inline styles for `minWidth={0}` or `minWidth={false}` ([#6373](elastic/eui#6373)) - `EuiFacetButton` no longer reports type issues when passing props accepted by `EuiButton` ([#6373](elastic/eui#6373)) Co-authored-by: Constance Chen <constance.chen@elastic.co>
Summary
This PR adds an optional
color
prop toEuiBeacon
component and closes #6315.General checklist
- [ ] Checked Code Sandbox works for any docs examples- [ ] Checked for accessibility including keyboard-only and screenreader modes