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

feat(tags): Filter option for No Tag #4253

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Simpler1
Copy link
Contributor

I moved this to a separate branch so that I could continue other work while we discuss any concerns.

Being able to filter for events that don't have a tag is very useful functionality.

The "0" is not the name of the tag, it's just the first tag in the dropdown list.

@IgorA100
Copy link
Contributor

Why did you close PR #4238 and now open a new one?
This way the point of the discussion is lost.

@Simpler1
Copy link
Contributor Author

I didn't expect this enhancement to be controversial. I originally made the change in my master branch. Since it didn't get merged, I needed to continue updating my master branch, so I created a dedicated branch for this enhancement. If you have any issues, please add them here so that we can discuss.

Do you now understand that the 0 just means that it's the first entry in the dropdown?

@connortechnology
Copy link
Member

The issue here is one of nitpicking and consistency as well as a misunderstanding of how arrays work in PHP, which is REALLY non-intuitive IMHO.

The value 0 does not in fact make it the first element. I know ... I know... the fact that you added it first, makes it the first element. php arrays actually mostly hashes (associative arrays) and remember the order added in addition to indexes.

Anyways if instead of $availableTags = array(translate('NoTag')); you did $availableTags = array(''=>translate('NoTag')); then the tests later on could be for empty string instead of the specific value 0, which is just a little more semantically nice to some people's way of thinking. In the past I have done the ==0 thing as well and have generally moved away from it. There have been actual technical reasons although I forget what they were now. So ideally it is about consistency with what has been done with other filtering inputs.

@Simpler1
Copy link
Contributor Author

This is an indexed array (not an associative array). With the change that you describe, it now has an index of [] instead of [0]. There is no way check for an index of "blank".

How do you propose to do the check?

Here's what the indexed array looks like with your proposed change:
(
[] => No Tag
[86] => Delete
[2] => Car
)

@IgorA100
Copy link
Contributor

IMHO the correct option is:
The Tags table should have a record 'Name' == '', what its ID will be does not matter.
And in this case there will be no need to pre-execute$availableTags = array(translate('NoTag'));|| $availableTags = array(''=>translate('NoTag'))
And then compare with ''

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.

3 participants