feat: modal implementation for data ingestion table #1244
Conversation
…com/splunk/addonfactory-ucc-generator into feat/spike-poc-dashboard-side-panel
rohanm-crest
changed the title
|
@rohanm-crest as there was data_ingestion_modal_definition.json created, there is also a need to adjust smoke tests accordingly instead of current lets make sure pipelines passed and i will than check all of it |
| } | ||
|
|
||
| table td:first-child:hover { |
There was a problem hiding this comment.
can we use some id for this one? table seems unsave? probably only table in this page but styles might be left without refreshing whole page and it will influence other tables.
|
some ideas regarding selector inside modal,
inside payload there are all data from table, thanks to that we can have all inputs from first column and add it into selector, Also it would be nice to have a separated selector (from definition.json) as we need to change json definition based on selector change) , if user clicks testInput2 we need to adjust search queries,
pros: |
| handleRequestClose={() => { | ||
| setDisplayModalForInput(null); | ||
| }} | ||
| title={`${displayModalForInput}`} |
There was a problem hiding this comment.
it should match figma
Data ingestion details (By input).
By input is changable and can be
By Input
By Source type
By Source
By Host
By Index
By Account
| `; | ||
|
|
||
| const ModalHeader = styled(Modal.Header)` | ||
| background-color: ${variables.backgroundColorHover}; |
There was a problem hiding this comment.
I am not sure the semantic name of the color backgroundColorHover applies to the non-hover background color
| `; | ||
|
|
||
| export const DataIngestionModal = (props: { | ||
| open: boolean | undefined; |
There was a problem hiding this comment.
| open: boolean | undefined; | |
| open?: boolean; |
| margin: 0px 10px; | ||
|
|
||
| .footerBtn:first-child { | ||
| justify-self: start; |
There was a problem hiding this comment.
can it be justify content space between for the parent?
There was a problem hiding this comment.
I have tried that but it's taking the full width for both the buttons
const StyledDiv = styled('div')`
display: flex;
justify-content: space-between;
margin: 0px 10px;
.footerBtn {
margin: 0 auto;
}
`;
| background-color: ${variables.backgroundColorHover}; | ||
| `; | ||
|
|
||
| const StyledDiv = styled('div')` |
There was a problem hiding this comment.
I would name this variable something more meaningful
| const StyledDiv = styled('div')` | ||
| display: grid; | ||
| grid-template-columns: 0.35fr 1fr; | ||
| margin: 0px 10px; |
There was a problem hiding this comment.
please avoid hardcoded value as much as possible
| margin: 0px 10px; | |
| margin: 0px ${variables.spacingSmall}; |
|
|
||
| const ModalWrapper = styled(Modal)` | ||
| width: 60%; | ||
| height: 80%; |
There was a problem hiding this comment.
does it work fine on small and huge screen sizes?
There was a problem hiding this comment.
Changed it to vh/vw respectivley
Issue number: ADDON-70993, ADDON-72969
Summary
Changes
User experience
Checklist
If your change doesn't seem to apply, please leave them unchecked.