-
Notifications
You must be signed in to change notification settings - Fork 22
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
feat: modal implementation for data ingestion table #1244
base: develop
Are you sure you want to change the base?
Conversation
…com/splunk/addonfactory-ucc-generator into feat/spike-poc-dashboard-side-panel
@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 |
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.
just a small thing i found after brief look
} | ||
|
||
table td:first-child:hover { |
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.
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: |
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.
Also lets remember to not marge it untill we align all dynamics part into this PR.
handleRequestClose={() => { | ||
setDisplayModalForInput(null); | ||
}} | ||
title={`${displayModalForInput}`} |
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
open: boolean | undefined; | |
open?: boolean; |
margin: 0px 10px; | ||
|
||
.footerBtn:first-child { | ||
justify-self: start; |
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.
can it be justify content space between for the parent?
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please avoid hardcoded value as much as possible
margin: 0px 10px; | |
margin: 0px ${variables.spacingSmall}; |
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.
Done
|
||
const ModalWrapper = styled(Modal)` | ||
width: 60%; | ||
height: 80%; |
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.
does it work fine on small and huge screen sizes?
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.
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.