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

added clipboard copy button #2051

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

thesayfulla
Copy link
Contributor

Description

Added a 'Copy' button for template context with unicode handling.

Fixes #1840

Hi @tim-schilling, please give me your feedback, thank you!

Copy link
Member

@tim-schilling tim-schilling left a comment

Choose a reason for hiding this comment

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

This is a nice start! I think we'd want a few more things before this could be merged.

  • The original issue calls out the fact that the context lists several dictionaries one after another with no delimiter. This PR may need to do something about that. The goal is to have something that can be copied and pasted into a shell (looking at the original issue). Please consider that workflow and let me know what you think.
  • Some selenium tests which may be annoying, but we need something to automatically test this

@thesayfulla
Copy link
Contributor Author

  • The original issue calls out the fact that the context lists several dictionaries one after another with no delimiter. This PR may need to do something about that. The goal is to have something that can be copied and pasted into a shell (looking at the original issue). Please consider that workflow and let me know what you think.

What do you think 'Copy' button would wrap these dictionaries inside a list format?

So, instead of copying:

{"data": "smth"}
{"data2": "smth2"}

It would copy:

[
    {"data": "smth"},
    {"data2": "smth2"}
]

@tim-schilling
Copy link
Member

I think that's better. The alternative is allowing them to copy each dict one at a time. This would require a small UI change to add some more vertical spacing between the dictionaries.


I wonder if we should use a dictionary with keys identifying where each of the contexts have come from.

That's a bit bigger of a change and should be a separate issue.


Last aside, some of the values are not valid primitives or common classes. They are things like "my_class" : <MyClass prop=Hello world other=10 ...> what do you suggest we do when that's the case? That value can not be pasted into a shell as is.

@thesayfulla
Copy link
Contributor Author

1. I think that's better. The alternative is allowing them to copy each dict one at a time. This would require a small UI change to add some more vertical spacing between the dictionaries.

Yes, I agree with you. My next question is: What should we do with the 'context processors'? Should I add a 'Copy' button as well? Initially, I thought we had the main context data, and I planned to implement 'Copy' as a list. That's why I'm bringing this up. Please let me know.

Screenshot 2025-01-12 at 20 12 38

2. I wonder if we should use a dictionary with keys identifying where each of the contexts have come from. That's a bit bigger of a change and should be a separate issue.

I'll search about it, i'm still learning about django-debug-toolbar :)

I'll try my best implement middleware for this, please give me your suggestion, how can I do it?)

3. Last aside, some of the values are not valid primitives or common classes. They are things like "my_class" : <MyClass prop=Hello world other=10 ...> what do you suggest we do when that's the case? That value can not be pasted into a shell as is.
Maybe repr() can help us. What do you think? Skipping data is another option, but I don't think it's a good idea.

@tim-schilling
Copy link
Member

Yes, I agree with you. My next question is: What should we do with the 'context processors'? Should I add a 'Copy' button as well? Initially, I thought we had the main context data, and I planned to implement 'Copy' as a list. That's why I'm bringing this up. Please let me know.

Oh dang. I'm sorry, I definitely didn't understand things properly. I thought the multiple dictionaries were coming from the context processors. They aren't. This is new to me.

Apparently the context for a template is stacked, which is why we have the list here. Though when the template is rendered, it just shows the flattened version. So, yes I think a list of dictionaries is appropriate here.

Though I think we may want to create an issue to inform the developer about this. The developer should be aware that multiple contexts are used for the view and what the flattened context dictionary was effectively used to render the template.

Maybe repr() can help us. What do you think? Skipping data is another option, but I don't think it's a good idea.

I actually think we can punt on this part. When the serializable branch gets merged, it's going to be converted to JSON and at that point, this will be "solved". Or at least decided for us.


It seems like the main change is the copy button (done, but needs tests) and displaying the contexts as a list.

@matthiask any opinions?

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.

Make copying context easier (for pasting into an editor or shell).
2 participants