DDF-3650 Create Search Form View and refactor existing functionality #3004
Conversation
| * | ||
| **/ | ||
| /*global define */ | ||
| define([ |
There was a problem hiding this comment.
I think you should use require here instead of define.
|
-1 at the moment. Please say "Use another Search Form" and refer to these as "Search Forms" rather than "Templates" -- also please make the cards and window larger with more "white space". They don't look selectable. The grey color used in "Basic" and "Text" is too light and looks greyed out. Please say "Search Form Browser" in the title bar (to align with future changes) |
|
@michaelmenousek addressed your comments. Here is an updated screenshot that also shows highlighting. The text is greyed out because nothing was highlighted in the previous screenshot. |
| @@ -14,5 +14,5 @@ | |||
| <div class="interaction-icon fa fa-file"> | |||
| </div> | |||
| <div class="interaction-text"> | |||
| Change Type | |||
| Use another Search Form | |||
There was a problem hiding this comment.
nit: Should A in "another" be capitalized since everything is title case?
| <div class="interaction interaction-type" title="Change the form used to construct the search." data-help="Change the form used to construct the search."></div> | ||
|
|
||
| <div class="interaction interaction-type-advanced interaction-form" title="Change the form used to construct the search." data-help="Change the form used to construct the search."> | ||
| <div class="interaction-icon fa fa-question-circle"></div> |
There was a problem hiding this comment.
nit: question mark seems a bit odd for "advanced" maybe something like <i class="fas fa-search"></i>
There was a problem hiding this comment.
yeah, seems more consistent with fa-search
| onRender: function () { | ||
| this.tabsContent.show(new SearchFormsView({ | ||
| model : this.model | ||
| })); |
There was a problem hiding this comment.
Should we have a triggerDropdown() here? Otherwise, if we select a template the dropdown remains open and feels awkward.
There was a problem hiding this comment.
I agree; looking into this now
|
@Variadicism fixed inconsistent spacing |
| } | ||
| user.savePreferences(); | ||
| this.triggerCloseDropdown(); | ||
| this.model.queryModel.trigger('closeDropDown'); |
There was a problem hiding this comment.
❓ It seems odd to me that this call is not in triggerCloseDropdown. triggerCloseDropdown doesn't seem to be used anywhere else, so I would advise moving the one method call in triggerCloseDropdown above this line and deleting the method. If I'm wrong and triggerCloseDropdown is used elsewhere, it seems like this line should be moved inside that method.
There was a problem hiding this comment.
moved inside of triggerCloseDropdown
| defaults: { | ||
| name :"A Search Form", | ||
| createdBy :"admin", | ||
| createdOn :"30 October 2018", |
There was a problem hiding this comment.
✏️ The space before each colon on the 3 lines above should be moved after the colon.
| @{customElementNamespace}search-form-system { | ||
|
|
||
| .search-form-system-title { | ||
| text-align:center; |
There was a problem hiding this comment.
✏️ 4 of the rules in here could use a space after the colon.
| this.model.queryModel.set('type', 'text'); | ||
| user.getQuerySettings().set('type', 'text'); | ||
| break; | ||
| } |
There was a problem hiding this comment.
This switch statement can be simplified to the following if statement:
const type = this.model.get('type');
if (type === 'basic' || type === 'text') {
this.model.queryModel.set('type', type);
user.getQuerySettings().set('type', type);
}
There was a problem hiding this comment.
we might want to leave this as a switch as more cases will likely be added in the future
|
@bdeining Thanks. I wasn't sure if spacing was considered significant enough for code review comments. If so, I've marked a couple other spots with inconsistent spacing, but they're ✏️. |
| margin: @minimumSpacing; | ||
| width: 200px; | ||
| height: 150px; | ||
| text-align:center; |
There was a problem hiding this comment.
Add vertical-align: top to keep the choices aligned correctly.
|
We should update the folder name for Also, update the |
|
Move the current |
|
@andrewkfiedler addressed your comments |
|
Thanks, I'll take another look, I got pulled away to work on something while I was reviewing. |
bdeining
changed the title
| childView: SearchFormView, | ||
| tagName: CustomElements.register('my-search-forms'), | ||
| className: 'is-list is-inline has-list-highlighting', | ||
| initialize: function(options) { |
There was a problem hiding this comment.
An easier way to pass these options on to the childView will be to use childViewOptions. We do something similar in
| @@ -0,0 +1,41 @@ | |||
| /** | |||
There was a problem hiding this comment.
We should update the filename to search-form.collection.js.
| this.add(new SearchForm({type: "basic", queryModel: options[0].queryModel})); | ||
| this.add(new SearchForm({type: "text", queryModel: options[0].queryModel})); | ||
| }, | ||
| getCustomQueryForms: function() { |
There was a problem hiding this comment.
Is this for a later PR to utilize?
There was a problem hiding this comment.
yes, there will be a follow-on PR that will utilize this
|
|
||
| module.exports = Backbone.Model.extend({ | ||
| defaults: { | ||
| name: "A Search Form", |
There was a problem hiding this comment.
We're going to need an id on these as well.
There was a problem hiding this comment.
The ID will be passed from the backend; these defaults will probably be removed in a future iteration
There was a problem hiding this comment.
My thoughts were that type is redundant with id. We would have reserved ids such as basic, text, and advanced while the others would be unique ids generated by the backend.
The reason I push for ids now is that the logic will be more complicated without them (we'll be checking type, and then checking id, when we could just do a check on id and when it doesn't match a reserved id we know it's custom).
| } | ||
|
|
||
| .search-form-label { | ||
| font-weight: bold; |
| } | ||
|
|
||
| @{customElementNamespace}search-form-loading { | ||
| padding: 2*@minimumSpacing; |
| }, | ||
| onRender: function(){ | ||
| this.listenTo(this.model, 'closeDropDown', this.triggerCloseDropdown); |
There was a problem hiding this comment.
listen to query model here instead
| * | ||
| **/ | ||
| /*global define, alert, setTimeout*/ | ||
| define([ |
There was a problem hiding this comment.
✏️ Could refactor this to use the more maintainable require notation
| * | ||
| **/ | ||
| /*global define, alert, setTimeout*/ | ||
| define([ |
There was a problem hiding this comment.
Approved pending:
- Feedback brought up during mob review meeting
- Removing nodes with null/empty values from the filter tree before submission to
CqlUtils
Regarding the later, if this change is going to take place globally (and not just apply to templates) then introducing a user preference for the inclusion of null or empty fields might be desirable.
| './query-custom.hbs', | ||
| 'js/CustomElements', | ||
| 'component/filter-builder-search-form/filter-builder.view', | ||
| 'component/filter-builder/filter-builder', |
There was a problem hiding this comment.
❓ Wait why is the view coming from component/filter-builder-search-form and the model from component/filter-builder? Where does component/filter-builder/filter-builder.view.js factor into this?
There was a problem hiding this comment.
The model need not be extended; the views use the same model but are presented a little differently. component/filter-builder/filter-builder.view.js is used in the query-advanced view
|
build now |
|
Internal build has been scheduled, your results will be available at build completion. |
|
Refer to this link for build results (access rights to CI server needed): |
|
build now |
|
Internal build has been scheduled, your results will be available at build completion. |
|
Refer to this link for build results (access rights to CI server needed): |
|
Looks good. There is a minor issue in IE11/Edge with a scroll bar on the text template. The window should be sized so no scrollbar appears. |
…odice#3004) * DDF-3650 Fixes some template bugs, adds caching to template fetching * DDF-3650 Code Review * DDF-3650 Create Template View and refactor existing functionality * Associated model * DDF-3650 is-bold
What does this PR do?
Create Template View and refactor existing functionality. Currently fixing some CSS issues with the dropdown view.
Who is reviewing it?
@mackncheesiest @Lambeaux
(please choose AT LEAST two reviewers that need to approve the PR before it can get merged)
Select relevant component teams:
https://github.com/orgs/codice/teams
Choose 2 committers to review/merge the PR.
(please choose ONLY two committers from below, delete the rest)
@andrewkfiedler
@jlcsmith
@millerw8
How should this be tested? (List steps with links to updated documentation)
Build / Install / Create Query / Mess around with templates
Any background context you want to provide?
What are the relevant tickets?
DDF-3650
Screenshots (if appropriate)
Checklist:
Notes on Review Process
Please see Notes on Review Process for further guidance on requirements for merging and abbreviated reviews.
Review Comment Legend: