Refactor Asset Inventory page #212436
Refactor Asset Inventory page #212436
Conversation
albertoblaz
added
release_note:skip
albertoblaz
force-pushed
the
asset-inv-refactor
branch
from
9035ca7 to
ada0b12
Compare
albertoblaz
force-pushed
the
asset-inv-refactor
branch
3 times, most recently
from
eeea887 to
c50144e
Compare
| @@ -498,14 +442,12 @@ export const AllAssets = ({ | |||
| showTimeCol={false} | |||
| settings={settings} | |||
| onFetchMoreRecords={loadMore} | |||
| externalControlColumns={externalControlColumns} | |||
| rowAdditionalLeadingControls={externalControlColumns} | |||
There was a problem hiding this comment.
externalControlColumns was deprecated and rowAdditionalLeadingControls was encouraged instead
| onResetColumns={onResetColumns} | ||
| /> | ||
| ); | ||
|
|
||
| const externalControlColumns: EuiDataGridControlColumn[] = [ | ||
| const externalControlColumns: RowControlColumn[] = [ |
There was a problem hiding this comment.
rowAdditionalLeadingControls's expected interface is RowControlColumn instead
| )} | ||
| /> | ||
| <FormattedMessage id={`${I18N_PREFIX}.allAssets.title`} defaultMessage="All Assets" /> | ||
| <TechnicalPreviewBadge /> |
There was a problem hiding this comment.
<EuiBetaBadge> stuff was encapsulated in <TechnicalPreviewBadge>
| showFullScreenButton | ||
| // showKeyboardShortcuts |
There was a problem hiding this comment.
Removed unnecessary props that default to true
| }; | ||
|
|
||
| return { | ||
| wrapperHeight: getWrapperHeight(), | ||
| mode: isVirtualizationEnabled ? 'virtualized' : 'standard', | ||
| }; | ||
| }, [pageSize, height, filters?.length, hasDistributionBar]); | ||
| }, [pageSize]); |
There was a problem hiding this comment.
wrapperHeight function was simplified since <AllAssets> does not receive props that alter its height unlike in Findings
| export const AllAssets = ({ | ||
| nonPersistedFilters, | ||
| height, | ||
| hasDistributionBar = true, | ||
| createFn, | ||
| ...rest | ||
| }: AllAssetsProps) => { |
There was a problem hiding this comment.
<AllAssets> does not need any prop so they're all gone now
There was a problem hiding this comment.
This file was only imported by use_asset_inventory_data_table so getDefaultQuery now lives there
There was a problem hiding this comment.
We forgot to delete the mocked data after the backend integration work
| @@ -69,6 +65,6 @@ export const AssetInventorySearchBar = ({ | |||
|
|
|||
| const getContainerStyle = (theme: EuiThemeComputed) => css` | |||
| border-bottom: ${theme.border.thin}; | |||
| background-color: ${theme.colors.body}; | |||
| background-color: ${theme.colors.backgroundBaseSubdued}; | |||
There was a problem hiding this comment.
theme.colors.body was deprecated
There was a problem hiding this comment.
Most constants are centralized here to have consistent values i.e.
- All local storage keys starting with same prefix
- All test subj with similar naming
- All query keys living in one place to make sure their values are different
- etc
|
Pinging @elastic/kibana-cloud-security-posture (Team:Cloud Security) |
albertoblaz
force-pushed
the
asset-inv-refactor
branch
from
c50144e to
ba15210
Compare
💚 Build Succeeded
Metrics [docs]Async chunks
Unknown metric groupsReferences to deprecated APIs
History
cc @albertoblaz |
| onResetFilters: () => void; | ||
| docsUrl?: string; | ||
| }) => { | ||
| export const EmptyState = ({ onResetFilters }: { onResetFilters: () => void }) => { |
There was a problem hiding this comment.
EmptyState is generic and i suggest to save this type of naming to generic components. this one is a wrapper of asset inventory around eui empty state components. i suggest to name file and component as asset inventory empty state
There was a problem hiding this comment.
Looks good! regarding our naming conventions and what I’m trying to do with the flyout:
Everything I place in the components folder is intended to be as generic as possible, like a package of reusable components that could be used anywhere in Kibana. Typically, these are enhancements or wrappers around EUI’s base components.
For domain-specific wrappers around generic or EUI components, I append the domain name to the component name. For example, I’d use names like AssetInventoryFilters or AssetInventorySearchBar in this case.
Of course, this is a subjective approach, so it's your call if you like or not. We can also take this offline as a group to decide if its something we want to try and stick to.
Interesting. This is something I wanted to bring up because I've seen inconsistencies in my code and in existing modules. I like a lot the idea of separating generic <-> domain-specific and prefixing the domain name. I just think Regarding hooks, I tried to separate them from components but maybe specific hooks should be co-located (next to the components that use them) while generic hooks or those that are used top-level might be placed at a top-level |
The hooks example you gave is exactly what i would suggest for the components as well. its much easier to do this when we have good folder structure for the UI itself. this is my approach, what makes the most sense to me at least. Obviously not all components require a generic component in the component folder. the |
Thanks @albertoblaz, I like the proposal of differentiating domain-specific components, it's a part of scalign the project, we started with a simpler structure by grouping only by "File Types" and now we seek to improve the maintainability, I just think we should decide on a consistent and scalable file structure and naming conventions to better differentiate domain-specific components from reusable ones. For example, we should be consistent about adding the So I guess the separation and naming should take into account if they are reusable outside the I wrote what could be a proposal for a follow-up PR focused on being consistent in structuring for whether the file is related to a domain or is generic, and I would like to hear more thoughts or suggestions on this approach. I also added some open questions while reviewing it to discuss: |
|
Maybe this will clarify my intention better: https://github.com/alan2207/bulletproof-react/blob/master/docs/project-structure.md The idea is that we have generic shared folders, which do not contain domain knowledge, including an each folder in Asked some LLM to implement this structure to our files, its pretty good, maybe hallucinated a little, and the files themselves might not be named the best. but this is a general structure that we can aim for and fine tune: |
|
@JordanSh thanks for the example, I understand it better now, I think the proposed approach works better as it still prioritizes colocation, 🛳️-it |
Summary
Refactors code in Asset Inventory page for simplicity and consistency.
Changes
<AllAssets>page, removed unused props, renamed variables, etc...<TechnicalPreviewBadge>Also, this PR deletes the mocked data that was used before integrating the UI with the backend.
Questions
localized_strings.ts?Out of scope
Hooks in
hooks/use_asset_inventory_data_tableand field selector components were all duplicated from the CSP plugin. I haven't put effort in refactoring those since we'll need to remove the duplication and make them reusableChecklist
release_note:breakinglabel should be applied in these situations.release_note:*label is applied per the guidelinesRisks
No risk since code is still hidden behind the Enable Asset Inventory advanced setting and the beta Cloud Asset integration must be installed.