Relevance and other composer tweaks #96
Conversation
and display sort fields dropdown in order used
when adding, clicking the icon adds it directly; but on palette clicking opens popup; and popup button gets correct configurable message
There was a problem hiding this comment.
The relevance filter works great 👍
Granted we should also put more work into the UX for the filters but that can be addressed later on.
However, I do have issue with the popup and the way it's implemented. There are CSS issues (see the white line on top of the title in the screenshot + things not aligned) and it does weird rendering when you stop hovering the information icon (see gif)
In order to not drag down the main feature, I suggest to remove commits related to the popover from this PR and open a separate one.
| if (match) return true; | ||
| fieldNum++; | ||
| if (!item.hasOwnProperty(field) || !item[field]) return false; |
There was a problem hiding this comment.
I know I'm a pain in the b** but could you use curly brackets for consistency please? :)
There was a problem hiding this comment.
it adds 6 lines making the code in my view a lot harder to follow. there is a reason languages allow one-line if consequents to be inlined :) . i've done it as you request but hope you see the light and conclude the verbosity is worse than the inconsistency!
| let text = item[field]; | ||
| if (!text.toLowerCase) { | ||
| text = JSON.stringify(text).toLowerCase(); | ||
| } else { | ||
| text = text.toLowerCase(); | ||
| } | ||
| return match || text.indexOf(part) > -1; | ||
| let index = text.indexOf(part); | ||
| if (index == -1) return false; |
| "pos", index, "/", text.length, | ||
| ":", item.relevance, "+=", score); | ||
| } | ||
| */ |
There was a problem hiding this comment.
i think it's useful in case we need to revisit how/why the scoring function works. it has a comment explaining why it is there so i'd prefer to keep it.
| if (index == -1) return false; | ||
| // found, set relevance -- uses an ad hoc heuristic preferring first fields and short text length, | ||
| // earlier occurrences and earlier words weighted more highly (smaller number is better) | ||
| let score = fieldNum * (2 / (1 + wordNum)) * Math.log(1 + text.length * index); |
There was a problem hiding this comment.
A bit complicated to understand at first but I like it! 👍
| Object.values($scope.state.currentOrder).forEach( it => { | ||
| $scope.state.currentOrderValues.push(it); | ||
| $scope.state.currentOrderFields.push(it.field); | ||
| }); |
There was a problem hiding this comment.
Instead of creating multiple objects and arrays, you can have only one array containing sort object that you update with Array.splice() and Array.unshift(). This array can then be use to generate the multi sort filter + the display on the template.
That would be more efficient that what we currently have
There was a problem hiding this comment.
it's O(n) so i don't think there's any major efficiency savings to be had. anyway n=5 so it's moot. the ugliness is around the multiple different lists but when i tried to use a single more sophisticated object angular didn't detect it was changing so seemed simplest to generate the exact different data structures we want. i could try to revise them each but that feels more brittle. so not worth changing in my view.
| <p><i class="mini-icon fa fa-fw fa-file-zip-o"></i> {{item.containingBundle}}</p> | ||
| <p ng-if="item.relevance"><i class="mini-icon fa fa-sort-numeric-asc"></i> Relevance score: {{ roundTwoDecimals(item.relevance) }}</p> | ||
| <p><i class="mini-icon fa fa-fw fa-file-zip-o"></i> {{popover.containingBundle}}</p> | ||
| <p ng-if="popover.relevance"><i class="mini-icon fa fa-sort-numeric-asc"></i> Relevance score: {{ roundTwoDecimals(popover.relevance) }}</p> |
There was a problem hiding this comment.
I don't think seeing the relevance number has any value for the user. In fact, we don't explain nor display what is the scale and order of this. I think we should not display this
There was a problem hiding this comment.
there is value in seeing a big jump for someone who uses this a lot, and i think it's more likely someone will appreciate this than resent it (but both are unlikely). leave it until/unless someone says they don't want it?
| @@ -341,7 +341,9 @@ <h4>No matching configuration</h4> | |||
| </br-collapsible> | |||
|
|
|||
| <!-- ENTITY LOCATION --> | |||
| <br-collapsible ng-if="[FAMILIES.ENTITY, FAMILIES.SPEC].indexOf(model.family) > -1" state="state.location.open"> | |||
| <ng-include src="'SpecEditorLocationSection.html'"></ng-include> | |||
There was a problem hiding this comment.
As we introduce more and more templates through $templateCache (which is great) we should then adopt a naming convention now instead of changing the name after and breaking downstream project.
Therefore I would propose <angular_component_type>/<component_name>/<template_id>.html. For example:
directive/spec-editor/section-location.htmlfor this particular templateview/main/graphical/footer.htmlfor a footer template in the viewmain.graphical
There was a problem hiding this comment.
agree w making these unique. i think it should definitely have the project name in it to make this happen.
don't think there's much value in component type. project_id/component_id/template_id would get my vote.
There was a problem hiding this comment.
suggest we do this as a separate PR however!
|
|
||
| <!-- ENTITY POLICIES --> | ||
| <br-collapsible ng-if="[FAMILIES.ENTITY, FAMILIES.SPEC].indexOf(model.family) > -1" state="state.policy.open"> | ||
| <ng-include src="'SpecEditorPoliciesSection.html'"></ng-include> |
|
|
||
| <!-- ENTITY ENRICHERS --> | ||
| <br-collapsible ng-if="[FAMILIES.ENTITY, FAMILIES.SPEC].indexOf(model.family) > -1" state="state.enricher.open"> | ||
| <ng-include src="'SpecEditorEnrichersSection.html'"></ng-include> |
| </br-collapsible> | ||
| </script> | ||
|
|
||
| <ng-include src="'SpecEditorOtherSections.html'"></ng-include> |
also add message for freeform tile
|
everything addressed @tbouron . fixed the two popover issues also, good spot there. also added a message for use with freeform tiles and tidied that. |
|
LGTM. There are couple of thing I want to change but I'll do this in a separate PR shortly. Merging this in the meantime |
Several things to make composer easier to use:
Below is a sample screen shot, for a downstream project that uses the UI (and has a lot of types), but the mods all apply to the stock brooklyn UI (though only really useful with a big catalog!):