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

Support a "None" option for the top term #1090

Merged
merged 2 commits into from
Jan 21, 2016
Merged

Conversation

benlk
Copy link
Collaborator

@benlk benlk commented Jan 21, 2016

Changes

  • adds 'none' option to largo_top_tag_display metabox function
  • changes the validation function for the top term from intval to sanitize_key because "None" cannot have an id of '0' because '0' and 0 and '' are falsy and therefore _largo_meta_box_post_save will remove the option instead of saving it
  • change largo_top_term so that it outputs '' if the top term option is 'null'

Why

For #1082 and RNS-127.

- adds 'none' option to largo_top_tag_display metabox function
- changes the validation function for the top term from `intval` to `sanitize_key` because "None" cannot have an id of `'0'` because `'0'` and `0` and `''` are falsy and therefore _largo_meta_box_post_save will remove the option instead of saving it
- change largo_top_term so that it outputs `''` if the top term option is `'null'`
@benlk benlk added the priority: high Either blocks work on a priority-normal task or a solution here informs other work. label Jan 21, 2016
@benlk benlk added this to the 0.5.5 - Story Elements milestone Jan 21, 2016
@benlk
Copy link
Collaborator Author

benlk commented Jan 21, 2016

Tests wouldn't run on this one on my machine, and I don't know if that's a problem with my vagrant or with our test rig or with this pull request. Please wait for Travis.

@benlk
Copy link
Collaborator Author

benlk commented Jan 21, 2016

Tests ran and passed on Travis, so 👍

See some issues in INN/deploy-tools for why I think tests aren't running on vagrant.

@benlk
Copy link
Collaborator Author

benlk commented Jan 21, 2016

When this merges to develop, merge the largo-1082-top-term-fixes branch of RNS into its theme and redeploy.

benlk added a commit that referenced this pull request Jan 21, 2016
Support a "None" option for the top term
@benlk benlk merged commit ac24c17 into develop Jan 21, 2016
@benlk benlk deleted the 1082-no-top-term-option branch February 3, 2016 19:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
priority: high Either blocks work on a priority-normal task or a solution here informs other work.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants