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

Select2 Upgrade/Exclude List #855

Merged
merged 39 commits into from Jun 28, 2016

Conversation

Projects
None yet
2 participants
@chacha
Copy link
Contributor

commented Jun 8, 2016

Moves towards using <select> boxes in the entire exclude interface.

@lukecarbis Please give feedback on any bugs you see.

See #852

@chacha chacha added the bug label Jun 8, 2016

Luke Carbis added some commits Jun 14, 2016

Luke Carbis
Luke Carbis
@lukecarbis

This comment has been minimized.

Copy link
Contributor

commented Jun 14, 2016

Some issues I've found:

  • Context Menu
    • Top level items should be bold
    • Filtering by top level items doesn't show the item pre-selected in the menu
    • Indents, instead of dashes, for secondary level items
    • Items with the same name (e.g. Installer > Plugins and Editor > Plugins) cause problems
  • Need to strip params from the _wp_http_referer before the form is submitted, otherwise you end up with really long URLs really quick
  • Exclude Rules default state should be "Any Author", "Any Context", etc.
  • Exclude Rules don't actually have any effect (Tested with Context: Comments and Action: Approved)
  • IP Address field
    • Too high (doesn't match other fields)
    • Highlight state doesn't match
    • Doesn't allow inputting custom IPs
return $output;
}
public function render_as_table() {

This comment has been minimized.

Copy link
@lukecarbis

lukecarbis Jun 14, 2016

Contributor

Rename to render_fields_table()

This comment has been minimized.

Copy link
@lukecarbis

lukecarbis Jun 14, 2016

Contributor

Docblock

*
* @return string
*/
public function render_all() {

This comment has been minimized.

Copy link
@lukecarbis

lukecarbis Jun 14, 2016

Contributor

Rename to render_fields()

*
* @return void
*/
public function __construct() {

This comment has been minimized.

Copy link
@lukecarbis

lukecarbis Jun 14, 2016

Contributor

Not required, remove.

* @param string $field_type The type of field being rendered.
* @param array $original_args The options for the field type.
*/
public function render_field( $field_type, $original_args ) {

This comment has been minimized.

Copy link
@lukecarbis

lukecarbis Jun 14, 2016

Contributor

Change $original_args to $args

return $output;
}
public function prepare_data_string( $data ) {

This comment has been minimized.

Copy link
@lukecarbis

lukecarbis Jun 14, 2016

Contributor

Docblock

This comment has been minimized.

Copy link
@lukecarbis

lukecarbis Jun 14, 2016

Contributor

Rename to prepare_select2_data_string? Not 100% clear what this method does.

This comment has been minimized.

Copy link
@chacha

chacha Jun 18, 2016

Author Contributor

This method prepares the HTML data attributes key/value pairs. For instance, if you have an array with array( 'my-key' => 'my-value' ) it will turn it into an HTML data attribute: data-my-key="my-value".

This is used to output all of the data attributes on the form elements.

'data' => array(
'group' => 'connector',
'placeholder' => esc_html__( 'Any Author or Role', 'stream' ),
'nonce' => esc_attr( wp_create_nonce( 'stream_get_users' ) ),

This comment has been minimized.

Copy link
@lukecarbis

lukecarbis Jun 14, 2016

Contributor

Is this creating a nonce per input? We only want one nonce per form.

This comment has been minimized.

Copy link
@lukecarbis

Luke Carbis and others added some commits Jun 14, 2016

Refactor Form Generator
Change render_all to render_fields
Change render_as_table to render_fields_table
Consolidate render_field wp_parse_args calls
Rename prepare_data_string to prepare_data_attributes_string
Add documentation
@chacha

This comment has been minimized.

Copy link
Contributor Author

commented Jun 20, 2016

@lukecarbis There were a couple incongruencies that I was unable to figure out.

Need to strip params from the _wp_http_referer before the form is submitted, otherwise you end up with really long URLs really quick

After monitoring the _wp_http_referer parameter, it never is any longer than /wp-admin/admin.php?page=wp_stream_settings.

Top level items should be bold
Indents, instead of dashes, for secondary level items

Looking at the exclude page, I do not see the problems you are talking about. See below:

screen1

I'll be looking to see if there is a way to reproduce the problems you're finding.

chacha and others added some commits Jun 20, 2016

Filter by ID instead of text
This makes it simpler to grab a top level item and its children, as the
ID always contains the top level item.
This doesn’t work for ‘Sites’ as the ID for sites is actually blogs.
Luke Carbis
Allow for tag creation
Removes clearAll button due to CSS issues.
Remove multiple attribute
Processing prevents multiple values
Luke Carbis
@lukecarbis

This comment has been minimized.

Copy link
Contributor

commented Jun 22, 2016

In regards to the _wp_http_referer param, you can replicate by filtering by action, then changing the action filter and pressing the Filter button again. Repeat 4 - 5 times and you will get a URL too long error.

@lukecarbis

This comment has been minimized.

Copy link
Contributor

commented Jun 22, 2016

In regards to the Top level items should be bold, Indents, instead of dashes… This occurs in the filters on the Stream screen.

@lukecarbis

This comment has been minimized.

Copy link
Contributor

commented Jun 27, 2016

@chacha Please review all the code, and test. This should now fix everything.

@chacha

This comment has been minimized.

Copy link
Contributor Author

commented Jun 27, 2016

@lukecarbis Looks great.

@lukecarbis lukecarbis merged commit fe9a17d into develop Jun 28, 2016

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@lukecarbis lukecarbis deleted the bugfix/issue-852 branch Jun 28, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.