-
Notifications
You must be signed in to change notification settings - Fork 98
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
remove button toggle and add stop button #623
remove button toggle and add stop button #623
Conversation
Signed-off-by: Kavitha Conjeevaram Mohan <[email protected]>
Codecov Report
@@ Coverage Diff @@
## main #623 +/- ##
=========================================
Coverage 54.70% 54.71%
Complexity 283 283
=========================================
Files 240 240
Lines 7778 7781 +3
Branches 1634 1636 +2
=========================================
+ Hits 4255 4257 +2
- Misses 3349 3350 +1
Partials 174 174
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
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.
Question: do we need to update cypress tests/jest tests, and update snapshots for these changes?
oh yes, a small change is required in cypress. i'll update it |
Signed-off-by: Kavitha Conjeevaram Mohan <[email protected]>
@@ -400,7 +400,6 @@ describe('Switch on and off livetail', () => { | |||
cy.wait(delay * 2); | |||
cy.get('.euiToastHeader__title').contains('On').should('exist'); | |||
|
|||
cy.get('[data-test-subj=eventLiveTail]').click(); |
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.
Why was this removed from cypress? Don't we switch on the live tail for cypress?
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 removed stop as an option from popover elements and kept it as a separate button, so removed this
setIsLiveTailOn(false); | ||
setLiveHits(0); | ||
setIsLiveTailPopoverOpen(false); | ||
if (isLiveTailOnRef.current) setToast('Live tail Off', 'danger'); |
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.
Question: does this needs to be a red colored toast. Would this give an erroneous impression to users?
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 was a change proposed by David to change it to red so that it would be different from the On toast message
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.
Minor edits.
* remove button toggle and add stop button Signed-off-by: Kavitha Conjeevaram Mohan <[email protected]> * update cypress Signed-off-by: Kavitha Conjeevaram Mohan <[email protected]>
Signed-off-by: Kavitha Conjeevaram Mohan [email protected]
Description
Replace EUI Button toggle with EUI button and add new stop button
Issues Resolved
[List any issues this PR will resolve]
Check List
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.