Skip to content
This repository was archived by the owner on Oct 8, 2021. It is now read-only.

Conversation

@brtn
Copy link
Contributor

@brtn brtn commented Oct 2, 2017

No description provided.

Copy link

@reinouts reinouts left a comment

Choose a reason for hiding this comment

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

Could you provide feedback to my comments?

}

//only one value is defined to filter? Then the other is always true
// if (this.falseitem ^ this.trueitem){
Copy link

Choose a reason for hiding this comment

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

Don't leave commented code in...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

for (var i = 0; i < this.menuItems.length; i++) {
var item = this.menuItems[i];

if (item.label == label) {
Copy link

Choose a reason for hiding this comment

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

A bit verbose?
item.set("checked", (item.label == label)) is more concise and would still be readable IMHO.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

"use strict";

return declare("TreeView.widget.Filters.AbstractFilter", null, {
fm: null,
Copy link

Choose a reason for hiding this comment

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

What's an fm?

dropDown : this.menu
});

if (this.filteranycaption)
Copy link

Choose a reason for hiding this comment

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

Use curly braces in if-else blocks.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

dojo.place(this.dropdown.domNode, this.fm.domNode);
},

getSearchConstraints: function () {
Copy link

Choose a reason for hiding this comment

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

You already extend AbstractFilter right? Shouldn't this function with default implementation be already there?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed.


},

free: function () {
Copy link

Choose a reason for hiding this comment

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

Why the empty implementation?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed.

},

getSearchConstraints : function() {
// var filters = (!this.filtersexclusive) ? this.filter.getFilters() : [];
Copy link

Choose a reason for hiding this comment

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

Delete commented line.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.



.gv_filter_dropdown_menu .dijitChecked::before {
content: √;
Copy link

Choose a reason for hiding this comment

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

should be configurable??

Copy link
Contributor

Choose a reason for hiding this comment

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

Disagree. This can be changed in a theme if someone wants to use something else. Plus, adding a symbol to ::before is not quite stable in Javascript.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually, this css file does not even affect current rendering of the widget, so maybe I should actually delete it entirely. The final styling should be defined in the theme.

@JelteMX JelteMX merged commit 77e55b6 into mendixlabs:master Oct 4, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants