Conversation
nursix
left a comment
nursix
left a comment
There was a problem hiding this comment.
Good start - but could not test-run it due to blocking JS errors (undeclared variables).
Not everything commented on needs to be fixed right away, but I have to hold off the merge until at least the JS blockers are fixed.
| * Update the widget options | ||
| */ | ||
| _init: function() { | ||
| self = this; |
There was a problem hiding this comment.
Undeclared variable self here, this should be const self = this;
You may want to use jshint or similar to catch such issues. Given that this script is in strict mode, this cannot possibly run as-is.
| getHtml() { | ||
| const parameterElement = $('<li class="dsform-parameter">'); | ||
|
|
||
| const header = this._renderHeader(data); |
There was a problem hiding this comment.
You care calling sub-method here with a parameter data, but data is never declared nor initialized here, which will throw immediately.
| const $el = $(this.element), | ||
| widgetID = $el.attr('id'); | ||
|
|
||
| window.addEventListener( |
There was a problem hiding this comment.
This event handler is never removed, so if the widget is instantiated more than once in the page, handlers will accumulate and run repeatedly. It makes therefore sense to add this as a name-spaced event in bindEvents, and remove the entire event name space in unbindEvents - both of which are called during refresh - and have no event handler binding during _init at all.
| # FORM(_id = 'test'), | ||
| # _class="ds-crud", | ||
| # ) | ||
| return DataSeriesForm().html('dsForm') |
There was a problem hiding this comment.
We do not use pascal-case in DOM IDs, so this should probably be 'ds-form' instead.
| _class='tiny primary button action-btn' | ||
| ), | ||
| FORM( | ||
| DIV(_id = "dsform-header"), |
There was a problem hiding this comment.
This is a hard-coded DOM ID, which is inconsistent with the following line, where you use a DOM ID derived from widget_id. Since any UI widgets shall be designed for multiple instances per page, hard-coded DOM IDs are prone to conflicts, and should not be used.
Alternatively, you can use a CSS class and bind events on that using the widget_id as reference frame, but this may come with performance trade-offs in some cases.
| class DataSeriesForm: | ||
| def html(self, widget_id): | ||
| widget = DIV( | ||
| BUTTON('Add measurement', |
There was a problem hiding this comment.
The button text is not internationalized, nor is it adaptive to the data model. Using T("Add Results") as a generic fallback might be a good idea - but in general, we need to make this use the CRUD strings of the target resource (here: med_analysis).
To do so, of course, you may have to pass in the resource for introspection.
|
|
||
| #dsform-items-selected { | ||
| border: 1px solid #e0e0e0; | ||
| padding: 10px; |
There was a problem hiding this comment.
Remember that you're inside a responsive CSS framework, where element and font-sizes may change per device and view port size/orientation.
This px-based sizing is good enough for a draft, but for production it needs to be validated across screen sizes and possibly changed into pt-based to make it adapt.
| padding: 0; | ||
| } | ||
|
|
||
| #dsform-collection .dsform-button-remove { |
There was a problem hiding this comment.
Note that the DOM IDs are (or should be) a variable, so using them to bind CSS is probably not the best strategy. Good enough for a first draft, but better to use CSS classes right away.
2 participants
Description
This is the first approach to implement the input form for storing measurements via Data Series Widget. The form loads and groups the parameters. Single parameters can be added to or removed from the form as well as whole groups. There is also a search bar to filter parameters and groups.
Todos