[SIEM] fix circular dependency #57729
Conversation
XavierM
added
bug
|
Pinging @elastic/siem (Team:SIEM) |
There was a problem hiding this comment.
Thank you for fixing this! I appreciate the thought that went into isolating these various constants/helpers into meaningful modules.
I smoke tested this locally, as well as ran the script to make sure things were green, and that if a new cycle was added it'd break properly.
I had one comment about renaming a type, but that's up to your discretion.
| EVENTS_VIEWER_ACTIONS_COLUMN_WIDTH, | ||
| } from '../constants'; | ||
|
|
||
| describe('helpers', () => { |
There was a problem hiding this comment.
🙏 thank you for adding tests!
There was a problem hiding this comment.
OK, just realized that you didn't add them per se, but: 🙏 thank you for not deleting them!
| export type ColumnId = string; | ||
|
|
||
| /** The specification of a column header */ | ||
| export interface ColumnHeader { |
There was a problem hiding this comment.
Would this be more accurate as ColumnHeaderOptions? I personally found it a little confusing that a ColumnHeader component receives a ColumnHeader prop.
|
|
||
| interface ColumneHeaderProps { | ||
| draggableIndex: number; | ||
| header: ColumnHeader; | ||
| header: IColumnHeader; |
There was a problem hiding this comment.
I don't have anything against naming things with a capital I at the front, but just wonder why we are changing it here and there? Should we be moving towards I with interfaces?
If it's just subjective and per person that's fine. Just curious.
There was a problem hiding this comment.
@FrankHassanabad I had a similar question, but I think it's because this file also defines a const ColumnHeader ? One should be a type and the other a value so it might not be strictly needed, but I'm guessing this is just a disambiguation. My comment about changing this to options: ColumnHeaderOptions is related.
There was a problem hiding this comment.
You are right @rylnd but I agree, I will rename it to ColumnHeaderOptions, that makes more sense to me too.
💚 Build SucceededHistory
To update your PR or re-run it, just comment with: |
* fix Timeline url dependency * fix circular dependency * rename ColumnHeader to ColumnHeaderOptions
* fix Timeline url dependency * fix circular dependency * rename ColumnHeader to ColumnHeaderOptions
Summary
I broke the circular dependency a while back, I am still surprised about it but I did. Therefore it is only fair that I had to fix it. So here we go!!!
Checklist
Delete any items that are not applicable to this PR.
Any text added follows EUI's writing guidelines, uses sentence case text and includes i18n supportDocumentation was added for features that require explanation or tutorialsThis was checked for keyboard-only and screenreader accessibilityThis renders correctly on smaller devices using a responsive layout. (You can test this in your browserThis was checked for cross-browser compatibility, including a check against IE11For maintainers
This was checked for breaking API changes and was labeled appropriately