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

764: highlight selected filter #765

Merged
merged 4 commits into from Jul 4, 2017
Merged

764: highlight selected filter #765

merged 4 commits into from Jul 4, 2017

Conversation

@toolstack
Copy link
Contributor

toolstack commented Jun 30, 2017

Resolves #764.

@toolstack toolstack requested a review from ocean90 Jun 30, 2017
@toolstack toolstack force-pushed the 764-hilight-selected-filter branch from 164739e to f9e0106 Jun 30, 2017
// Translators: %s is the total strings count for the current translation set.
$filter_links[] = gp_link_get( $url, sprintf( __( 'All (%s)', 'glotpress' ), number_format_i18n( $translation_set->all_count() ) ) );
$filter_links[] = gp_link_get( $url, $add_bold['open'] . sprintf( __( 'All (%s)', 'glotpress' ) . $add_bold['close'], number_format_i18n( $translation_set->all_count() ) ) );

This comment has been minimized.

Copy link
@ocean90

ocean90 Jul 1, 2017

Member

gp_link_get() supports a third argument to pass additional attributes. Could we use that instead of the cryptic bold variables? :)

Something like this:

$is_current = array() === array_diff( $all_filters, $filters_and_sort ) || array() === $filters_and_sort
$filter_links[] = gp_link_get(
	$url,
	sprintf( … ),
	$is_current ? array( 'class' => 'current' ) : array()
);

This comment has been minimized.

Copy link
@toolstack

toolstack Jul 1, 2017

Author Contributor

Could, but is the tirnary operator any less cryptic?😉

This comment has been minimized.

Copy link
@ocean90

ocean90 Jul 2, 2017

Member

It's more about the bold_on and bold_off variables, at first view I had no idea what they are for.

This comment has been minimized.

Copy link
@toolstack

toolstack Jul 2, 2017

Author Contributor

Ok, I'll make some tweaks to it.

This comment has been minimized.

Copy link
@toolstack

toolstack Jul 3, 2017

Author Contributor

I've written the PR to use classes.

@toolstack toolstack force-pushed the 764-hilight-selected-filter branch 2 times, most recently from 237962f to db769ee Jul 3, 2017
'status' => 'current_or_waiting_or_fuzzy_or_untranslated',
);

array() === array_diff( $all_filters, $filters_and_sort ) || array() === $filters_and_sort ? $extra_classes = array( 'class' => 'filter-current' ) : $extra_classes = array();

This comment has been minimized.

Copy link
@ocean90

ocean90 Jul 3, 2017

Member

That's still not readable. Let's put the result of the array() === array_diff( $all_filters, $filters_and_sort ) || array() === $filters_and_sort in a $is_current variable and pass $is_current ? array( 'class' => 'filter-current' ) : array(); to gp_link_get() instead of $extra_classes.

This comment has been minimized.

Copy link
@toolstack

toolstack Jul 4, 2017

Author Contributor

I updated the PR with a minor change as array( 'class' => 'filter-current' ) breaks the coding standard so I created a variable to hold the array instead.

@ocean90
ocean90 approved these changes Jul 4, 2017
@toolstack toolstack force-pushed the 764-hilight-selected-filter branch from 2307b20 to de2ba43 Jul 4, 2017
@toolstack toolstack merged commit 1f36c2b into develop Jul 4, 2017
2 checks passed
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
@toolstack toolstack deleted the 764-hilight-selected-filter branch Jul 4, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants
You can’t perform that action at this time.