Conversation
68c5758 to
b13df6a
Compare
|
@ivs-cetmix should be ready to test 👍 |
ivs-cetmix
left a comment
There was a problem hiding this comment.
Perfect, thank you @len-foss!
Here are some glitches I encountered during testing on MacOs 15.6 (24G84) in Safari Version 18.6 (20621.3.11.11.3)
Issue 1
Steps to reproduce (on runboat)
- In the second (text) key start typing "qwerty"
Expected behaviour
- The entire string can be typed in without loosing the focus
Current behaviour
- Need to click in the widget after each symbol is typed in. Eg "q" -> click, "w" -> click , ....
Issue2
Field properties drop down is rendered below the chatter buttons.
Video of both issues.
26e4630 to
8cd2914
Compare
|
@ivs-cetmix Easy point 2; I've added a CSS rule to increase the z-index. Good catch. So, point 1. I actually broke editing trying to do a "quick patch" of a very specific problem: if the user calls the record switcher while the recordset is dirty, the component does not go through |
ivs-cetmix
left a comment
There was a problem hiding this comment.
Functional review LGTM
@len-foss works now, thank you!
P.S. If you have LinkedIn we can promote this module to get more attention and have it merged faster 😄
| }; | ||
| static defaultProps = {}; | ||
|
|
||
| setup() { |
There was a problem hiding this comment.
Your createEditor() already has a great docstring. Adding the same for setup() and detect_dark_mode() helps future developers:
/**
- Setup lifecycle hooks and state initialization.
-
- Tracks record resId and resModel
-
- Binds editor creation on mount
-
- Cleans up editor on unmount
*/
setup() { ... }
- Cleans up editor on unmount
/**
- Detect if dark mode is active.
- Currently relies on cookie
color_scheme.
*/
detect_dark_mode() { ... }
There was a problem hiding this comment.
Currently, the code is pretty short, at barely a 100 lines of logic. Adding such fluff would only make it less readable, and would only serve a purpose if it was used by some automatic documentation generation tool, which aren't used within the project.
| this.state.value = new_value; | ||
| this.props.record.update({[this.props.name]: new_value}); | ||
| } catch (error) { | ||
| console.error("Error updating JSON value:", error); |
There was a problem hiding this comment.
Replace console.error with Odoo’s Notification service
this.env.services.notification.add(_t("Error updating JSON: ") + error.message, { type: "danger" });
| onChange: () => { | ||
| try { | ||
| const new_value = this.editor.get(); | ||
| this.state.value = new_value; |
There was a problem hiding this comment.
also update state with setter
this.state.value = { ...newValue };
|
This PR has the |
|
Hi @len-foss , could you please check the code review comment and reply or apply them? Would be really great to have your excellent module merged! |
|
Hi @ivs-cetmix done 👍 Thank you @ShehbajTechultra for the suggestions. |
|
@ShehbajTechultra could you please do an after-update review? Than you in advance! |
@ShehbajTechultra , perfect, thank you! |
|
@ivs-cetmix do you mean you prefer to have the commits squashed? |
|
@len-foss , yes, we need to squash commits so we have two - one per module. |
9fa5534 to
d700636
Compare
|
Hey @OCA/web-maintainers looks like we have a new cool widget ready to join the OCA family. Would appreciate your review) |
hbrunn
left a comment
There was a problem hiding this comment.
please check out https://github.com/OCA/web/blob/18.0/web_widget_x2many_2d_matrix/static/tests/web_widget_x2many_2d_matrix.test.js for an example of how easy you can have your code tested nowadays.
This is called by https://github.com/OCA/web/blob/18.0/web_widget_x2many_2d_matrix/tests/test_web_widget_x2many_2d_matrix.py
web_json_widget/__manifest__.py
Outdated
| # content from https://app.unpkg.com/jsoneditor@10.2.0/files/dist | ||
| # source: https://github.com/josdejong/jsoneditor?tab=readme-ov-file | ||
| # licenced under Apache-2.0 license | ||
| "/web_json_widget/static/src/lib/dark-theme.css", |
There was a problem hiding this comment.
those usually go into /static/lib
| // Improvements: add supportedOptions, ExtractProps... | ||
| }; | ||
|
|
||
| registry.category("fields").add("json_editor", JsonEditor); |
There was a problem hiding this comment.
I'd suggest
| registry.category("fields").add("json_editor", JsonEditor); | |
| registry.category("fields").add("json", JsonEditor); |
because then Odoo should pick the widget automatically, given there's not standard json widget
| this.resId = this.props.record.resId; | ||
| this.resModel = this.props.record.resModel; | ||
| if (this.editor) { | ||
| this.editor.set(this.props.record.data[this.props.name] || {}); |
There was a problem hiding this comment.
| this.editor.set(this.props.record.data[this.props.name] || {}); | |
| this.editor.set(this.state.value); |
There was a problem hiding this comment.
So, this one is pretty bad. Because of the very stupid OCA rule I squashed my commits, but basically if your recordset is dirty, using the record switcher makes it so that none of the widget lifecycle hooks are called. So, this.state.value is not updated, so you switch and the widget is not updated. I couldn't find anything that seemed to make sense and the odoo dev I asked was a clueless. This is inspired by how it's done by the mail JS code.
To clarify, onParched is a lifecycle hook but that is called continuously, whereas you'd want a 1-time one like onWillUpdateProps.
So, if there's a good explanation and solution, I'd like to know it.
|
|
||
| const isDarkMode = this.detect_dark_mode(); | ||
| if (isDarkMode) { | ||
| this.editorRef.el.classList.add("json-editor-theme-dark"); |
There was a problem hiding this comment.
I wonder why you don't do this in the template?
There was a problem hiding this comment.
Not sure exactly but the doc about the Json editor was a bit confusing to me, and I found confusing answers as to how to instantiate a dark version. I think the 'Odoo way' is to put that in a CSS without the dark mangling which is put into the dark assets bundle. As of now I find it simpler to keep it this way because I copy-pasted this from recommended setup, but it wouldn't be hard to change afterwards.
| onChange: () => { | ||
| try { | ||
| const new_value = this.editor.get(); | ||
| this.state.value = {...new_value}; |
There was a problem hiding this comment.
isn't it the point of useState that you don't need to update this manually? Actually, what use is the state proxy in this widget in the first place?
There was a problem hiding this comment.
You're right, it's not needed at all. I started by copying a basic widget, it's a 'PoC' remnant.
|
@hbrunn |
Add a Json editor widget.
Supports edits, dark mode (should work on OCA's version, since it relies on setting the
color_themecookie todark). Can even be put in a tree view, although you probably don't want to do that.This module uses the library jsoneditor.
It is released under the Apache-2.0 license.