Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Operator added to the widget #27

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Operator added to the widget #27

wants to merge 4 commits into from

Conversation

mustela
Copy link

@mustela mustela commented Apr 19, 2013

The operator was added to the widget, now you can choose between add/in

http://wordpress.org/support/topic/list-view-does-not-update-correctly

The operator was added to the widget, now you can choose between add/in

http://wordpress.org/support/topic/list-view-does-not-update-correctly
$this->selected_terms = explode( '+', $terms );
else
$this->selected_terms = explode( ',', $terms );

Copy link
Author

Choose a reason for hiding this comment

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

I'm not proud of these lines, since I think it can be written much better.

Copy link
Owner

Choose a reason for hiding this comment

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

So, what's stopping you? :)

Copy link
Author

Choose a reason for hiding this comment

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

Sunny days and lot of work! :P, but I'll improve it :)

Copy link
Owner

Choose a reason for hiding this comment

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

Can't argue with sunny days. ☀️

@scribu
Copy link
Owner

scribu commented Apr 21, 2013

The 'operator' field doesn't seem to have any effect in 'lists' mode.

'IN' => __( 'Or', 'query-multiple-taxonomies' )
),
'text' => false,
'desc' => __( 'Operator:', 'query-multiple-taxonomies' ),
Copy link
Owner

Choose a reason for hiding this comment

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

Operator: seems vague; it should either be Operator between terms: or Operator between taxonomies:.

@mustela
Copy link
Author

mustela commented Apr 24, 2013

@scribu do you have any other comment about the code?

@scribu
Copy link
Owner

scribu commented Apr 24, 2013

Yes, I made a comment above:

The 'operator' field doesn't seem to have any effect in 'lists' mode.

@mustela
Copy link
Author

mustela commented Apr 25, 2013

oh sorry, didn't see that. Yes, you are right there, I'll try to improve that.

@scribu
Copy link
Owner

scribu commented Apr 25, 2013

I'm also wondering if it would be better to split the modes into separate widgets:

  • Taxonomy Drilldown: Lists (with operator)
  • Taxonomy Drilldown: Checkboxes (with operator)
  • Taxonomy Drilldown: Dropdowns (without operator, since there's only one term per taxonomy)

All 3 widget classes could inherit from a base class.

@mustela
Copy link
Author

mustela commented Apr 25, 2013

Yes, that could work. I was trying to add something like that with jquery, but I'm having issues trying to get values. Can't find the issue.

@@ -51,6 +52,17 @@ function add_script() {
$(this).sortable();
$(this).disableSelection();
});

$('#qmt_types').change(function(){
console.log($('#qmt_types').val());
Copy link
Owner

Choose a reason for hiding this comment

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

This shouldn't be part of the PR.

Copy link
Author

Choose a reason for hiding this comment

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

Yes, I just uploaded this changes so you can check the code. But don't pay attention to that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants