-
Notifications
You must be signed in to change notification settings - Fork 58
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
added icon for new tab #791
base: main
Are you sure you want to change the base?
Conversation
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.
could you add some description on the PR, what is the change?
also i remember log_explorer has tests, any tests changes?
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #791 +/- ##
==========================================
+ Coverage 43.66% 43.72% +0.06%
==========================================
Files 312 313 +1
Lines 18587 18629 +42
Branches 4528 4534 +6
==========================================
+ Hits 8116 8146 +30
- Misses 9921 9932 +11
- Partials 550 551 +1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
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.
Overall looks good, can you provide screenshot/was this change went over by UX?
added the icon for top tabbing which was initially just a text @derek-ho |
@@ -25,11 +25,17 @@ | |||
} | |||
} | |||
|
|||
.logs-eui-icon { | |||
display: inline-block; | |||
vertical-align: text-bottom; |
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 aligned to text-bottom?
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.
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.
had a discussion with Eric and changed the icon to use button
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.
Hey Sumuk, could you attach a screenshot in description for new look and feel?
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.
Signed-off-by: sumukhswamy <sumukhhs@amazon.com>
Signed-off-by: sumukhswamy <sumukhhs@amazon.com>
Signed-off-by: sumukhswamy <sumukhhs@amazon.com>
Signed-off-by: sumukhswamy <sumukhhs@amazon.com>
Signed-off-by: sumukhswamy <sumukhhs@amazon.com>
Signed-off-by: sumukhswamy <sumukhhs@amazon.com>
Signed-off-by: sumukhswamy <sumukhhs@amazon.com>
Signed-off-by: sumukhswamy <sumukhhs@amazon.com>
This PR is stalled because it has been open for 30 days with no activity. |
Description
wrote react code for adding an icon for "Add new" whih adds a new tab in the logs explorer front end.
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.