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

Add new element JSONEditor #1474

Merged
merged 12 commits into from
Aug 25, 2023
Merged

Add new element JSONEditor #1474

merged 12 commits into from
Aug 25, 2023

Conversation

natankeddem
Copy link
Contributor

This element introduces a JSON targeted graphical text editor. It is useful for editing configuration files which maybe in JSON format or forming JSON data to pass to other elements.

Configuration and data is handled similarly to chart and echart utilizing a single dictionary properties. I have implemented on_select and on_change events to facilitate data return from the editor element.

I haven't done much testing on this element yet but it appears to have some functionality as implemented.

@natankeddem
Copy link
Contributor Author

I think this is probably ready for a code review. One feature which hasn't been implemented yet is themes. I am not sure how the maintainers would like to structure the css themes provided with the library. These would allow for light mode and dark mode applications. Would these be stored in /nicegui/nicegui/static or with the library itself? There wasn't an obvious way of loading additional css files like the various libraries variables in Element for javascript files so what would be most NiceGUI way of loading additional css files be?

@falkoschindler falkoschindler self-requested a review August 24, 2023 12:48
Copy link
Contributor

@falkoschindler falkoschindler left a comment

Choose a reason for hiding this comment

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

@natankeddem Thanks for this pull request! Even though it seems to be a rather specific UI element, it might come in handy for development and prototyping UIs when working on JSON configurations.

I reviewed your code and did some improvements here and there. There's just one concern remaining: I had to limit the select event types to "text", "key" and "value" because there's also "multi" and possibly others that don't map to the current Python events. So I wonder if it was more useful to simply send generic select events and keep event arguments in a dictionary so that we don't have to have classes for every event. Or is it better to use dedicated event argument classes with proper typing? What do you think?

@natankeddem
Copy link
Contributor Author

If you think a generic dictionary is acceptable, I am in agreement. I think it is better to have a single on_select callback if possible. In my own usage I only find the path when type != "text" to be useful, but others might have other use cases that require other variables. The library documentation does not do a good job explaining the variables passed to the onSelect event and how they differ depending on the editor state.

It appears like this library is a reboot of a previous project and is still being actively changes so it might be best to allow more flexibility. I understand the advantage of the event types with type hinting and being generally more explicit which is obviously a positive. Those positives need to be weighed against complexity and maintainability of the codebase in my opinion.

I can alter the PR to make the generic dictionary change you have proposed. I will change to a single on_select callback and will pass it the sender, client and selection variables in a JsonEditorSelectEventArguments dataclass argument. In this case the selection variable will be the content of the onSelect event's selection argument. Does this sound reasonable?

@falkoschindler
Copy link
Contributor

@natankeddem That sounds great! Thanks!
Especially your estimation about the project status makes me also lean towards more generic event arguments. And I like the name selection as a container for all selection-related properties.

@natankeddem
Copy link
Contributor Author

If you like that height as a default we could put that in the CSS if you want? Also, did you want to do anything with trying to figure out utilizing the built in theme css file handling? Or we could push that to a later PR if needed.

@falkoschindler
Copy link
Contributor

You mean h-80? I just noticed that we might be better of not setting any width or height in nicegui.css for the JSON editor. In contrast to, e.g., AG Grid the JSON editor comes with a very useful default height depending on its content. And the width automatically shrinks to 100% for small containers. (The settings in nicegui.css are basically to avoid confusion if the user creates an element that has zero width or height until the dimensions are set explicitly.) So I would remove the nicegui-json-editor class and the corresponding CSS definition as well as the h-80 in the documentation.

The built-in theme handling sounds like a good subject for another PR. To be honest, I haven't looked into it yet.

@falkoschindler
Copy link
Contributor

Thanks again for your contribution, @natankeddem! Looks really nice. 😊
I'll merge into main and prepare the release of v1.3.12.

@falkoschindler falkoschindler merged commit 9904164 into zauberzeug:main Aug 25, 2023
1 check passed
@falkoschindler falkoschindler added this to the 1.3.12 milestone Aug 25, 2023
@falkoschindler falkoschindler added the enhancement New feature or request label Aug 25, 2023
@natankeddem
Copy link
Contributor Author

A pleasure as always, thank you @falkoschindler !

@rodja
Copy link
Member

rodja commented Aug 26, 2023

Documentation and demo is live at https://nicegui.io/documentation/json_editor

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

Successfully merging this pull request may close these issues.

None yet

3 participants