Skip to content
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: actions on log messages #2572

Merged
merged 12 commits into from
Dec 9, 2023

Conversation

akash-ramaswamy
Copy link
Contributor

Initial implementation of actions on log messages - Copy to Clipboard.

@akash-ramaswamy
Copy link
Contributor Author

#2571

@amir20
Copy link
Owner

amir20 commented Dec 7, 2023

I reviewed the PR. A couple of comments:

  1. Seems like the hover is on top of the text not row. This is confusing because it seems to disappear easily.
  2. Some performance issues because you are using javascript to do hover. I'll comment on PR on this.
  3. Remembering why I didn't do this, I think you might need a better solution when the end of text would make the row jump down. I have attached a screenshot.
Screenshot 2023-12-07 at 11 51 34 AM

With above example, when not hovered, it's one line. But when hovered, it is 2 lines.

assets/components/LogViewer/ComplexLogItem.vue Outdated Show resolved Hide resolved
<carbon:row-collapse v-else />
</div>
<div
@click="logEntry.copyLogMessageToClipBoard()"
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

HTML could be cleaned up. You don't need parent div. Just put it on the link.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried a different solution now. I have moved the position of the copy button to end, making this stay constant.

assets/components/LogViewer/LogViewer.vue Outdated Show resolved Hide resolved
assets/models/LogEntry.ts Outdated Show resolved Hide resolved
@akash-ramaswamy
Copy link
Contributor Author

Hi @amir20 ! Thanks for the review. I will get back with the changes and improvements needed!

  1. Remembering why I didn't do this, I think you might need a better solution when the end of text would make the row jump down. I have attached a screenshot.

This is a valid point. I must also think of some way of solving this 🤔

@akash-ramaswamy
Copy link
Contributor Author

Hi @amir20 I have moved the position of the copy button to the end, so that it doesn't disturb the UI. And in future, we can make this a menu type vertical-three-dot menu, to make this a small dropdown to add more actions against log messages.

Screen.Recording.2023-12-08.at.6.20.31.PM.mov

@amir20
Copy link
Owner

amir20 commented Dec 8, 2023

Hi @akash-ramaswamy. This is going the right direction I think. I was actually thinking something similar where you move the icons to the right. One thought though, I noticed you have added padding to the right. This makes it so that there is less space. The most common use case is to look at logs, so we shouldn't sacrifice that width in my opinion.

When I was thinking about this, I was thinking of using position: absolute. More examples https://tailwindcss.com/docs/position. Then do right-0 and only show on hover. This should make it so that the icons show on top of the content and therefore not take any space.

Lastly, I feel like if there was a delay to show the icon would be great. But I believe that is not easily possible. You can probably use something like transition with delay. I noticed when moving the mouse the icon kept popping up and it felt weird to distracting. But this is minor.

@akash-ramaswamy
Copy link
Contributor Author

akash-ramaswamy commented Dec 8, 2023

Hi @akash-ramaswamy. This is going the right direction I think. I was actually thinking something similar where you move the icons to the right. One thought though, I noticed you have added padding to the right. This makes it so that there is less space. The most common use case is to look at logs, so we shouldn't sacrifice that width in my opinion.

When I was thinking about this, I was thinking of using position: absolute. More examples https://tailwindcss.com/docs/position. Then do right-0 and only show on hover. This should make it so that the icons show on top of the content and therefore not take any space.

Lastly, I feel like if there was a delay to show the icon would be great. But I believe that is not easily possible. You can probably use something like transition with delay. I noticed when moving the mouse the icon kept popping up and it felt weird to distracting. But this is minor.

I tried positioning the copy button. Its now on top of content as you are saying. But if content is more, its like overlapping a little bit, and I think thats okay since, we can lower opacity of icon when not hovered on.. your thoughts @amir20 ?

Screen.Recording.2023-12-08.at.11.02.10.PM.mov

@amir20
Copy link
Owner

amir20 commented Dec 8, 2023

Yea that looks better. You could also add a background color as the icon is pretty hard to notice on top of the text.

@akash-ramaswamy
Copy link
Contributor Author

Yea that looks better. You could also add a background color as the icon is pretty hard to notice on top of the text.

I chosen the text-primary, since it looks cool over log row's background 😎

Normal State

Screenshot 2023-12-09 at 4 32 56 AM

Hovered State

Screenshot 2023-12-09 at 4 32 56 AM

assets/components/LogViewer/CopyLogMessage.vue Outdated Show resolved Hide resolved
assets/components/LogViewer/CopyLogMessage.vue Outdated Show resolved Hide resolved
assets/components/LogViewer/CopyLogMessage.vue Outdated Show resolved Hide resolved
@amir20
Copy link
Owner

amir20 commented Dec 8, 2023

Looking good <3

how much work is left to finish it? There is also failed tests but i think you just need to update snapshot.

@akash-ramaswamy
Copy link
Contributor Author

Looking good <3

how much work is left to finish it? There is also failed tests but i think you just need to update snapshot.

i updated the snapshot, but I find that \\" is added before every attributes, which was not there earlier.

@amir20
Copy link
Owner

amir20 commented Dec 8, 2023

Make sure to rebase master. Vitest was updated.

1 similar comment
@amir20
Copy link
Owner

amir20 commented Dec 8, 2023

Make sure to rebase master. Vitest was updated.

@akash-ramaswamy
Copy link
Contributor Author

akash-ramaswamy commented Dec 9, 2023

Hi @amir20 ! I have updated the PR with the cleanups. but checking some tests are failing.

@akash-ramaswamy
Copy link
Contributor Author

Yup, test cases are passing!

@akash-ramaswamy
Copy link
Contributor Author

Once again, thanks for your time on reviewing @amir20 :)

Copy link
Owner

@amir20 amir20 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A couple quick fixes and ready to merge.

assets/components/LogViewer/CopyLogMessage.vue Outdated Show resolved Hide resolved
assets/components/LogViewer/CopyLogMessage.vue Outdated Show resolved Hide resolved
assets/components/LogViewer/CopyLogMessage.vue Outdated Show resolved Hide resolved
@akash-ramaswamy
Copy link
Contributor Author

I have at most extracted the classes which are not required for that CopyLogMessage component. Have a look @amir20 !

@amir20 amir20 merged commit e6fa8ce into amir20:master Dec 9, 2023
6 checks passed
@akash-ramaswamy akash-ramaswamy deleted the actions-log-messages branch December 9, 2023 14:12
@amir20
Copy link
Owner

amir20 commented Dec 9, 2023

I merged this. Do you have time fix the search mode? Or should I release first?

@akash-ramaswamy
Copy link
Contributor Author

akash-ramaswamy commented Dec 9, 2023

I merged this. Do you have time fix the search mode? Or should I release first?

Do you mean the log search mode @amir20 ? Im not able to get you.

@amir20
Copy link
Owner

amir20 commented Dec 9, 2023

Yea, I actually found a bug. Looks like when the text is too long the icon is cut off.

and back to the search, it probably should be all in one place before releasing.

Screenshot 2023-12-09 at 6 21 53 AM

The bug might be minor because as you see in the screenshot, it's because I have a lot of text. But I have attached the logs in case you want to test it.

Imgproxy cache log Dec 9.gz

@amir20
Copy link
Owner

amir20 commented Dec 9, 2023

I was thinking you can put all the icons in one place and then using v-if dynamically adjust them.

@amir20
Copy link
Owner

amir20 commented Dec 9, 2023

Oops more bugs when testing JSON...

Screenshot 2023-12-09 at 6 28 44 AM

@akash-ramaswamy
Copy link
Contributor Author

I will test this immediately and fix this asap @amir20 !

@amir20
Copy link
Owner

amir20 commented Dec 9, 2023

I'll hold off releasing. JSON usage is high.

For your testing, you can use https://github.com/amir20/echo which you can do docker run -i amir20/echo < data.json and pass any data you want. amir20/echo will randomly pick lines and print every second.

You should try to test all the different variations. I am pretty on weekends, but try to respond when I can.

I have created a bug so we don't keep discussing here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants