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

Added a New Card Filter For Labels #9

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

Conversation

nkuleedube
Copy link

No description provided.

@mostertb mostertb self-requested a review November 10, 2019 07:59
@mostertb
Copy link
Contributor

Hi @peace-d

Thank you for taking the time to submit this feature! We really do appreciate it.
My apologies on the delay in getting you feedback

I will be giving comments inline as well as on the your comment on the related issue but here is some general feedback:

Firstly, in terms of the feature that is discussed in #2, we are looking specifically to filter to cards that have at least one label that has a chosen name. Consider the following:
image
As you can see, Trello has allowed by to define two different labels with the name 'Alpha'. In this case, as a user I should only see 'Alpha` once as an option when setting up the filter and the filter should then include cards with either the Orange or the Yellow label in the above example.

Your code filters to cards that have a specific label (via filtering on the ID field). While not being explicitly what is described in #2, this is a useful too! Im happy with you to either change this PR to be in line with #2 or to change the display (and class name) to make it clearer that the filter provides the new functionality of filtering to a specific label. Let us know which you prefer.

Secondly, please could I ask that you run your code on a test board. One of the other recent PRs has resulted in the composer install needing a little more memory. If you're running into this issue, have a look at the (Memory Limit Errors)[https://getcomposer.org/doc/articles/troubleshooting.md#memory-limit-errors] section of the composer docs.

This PR is showing a lot of promise. I look forward to seeing your next push

'Select Label to filter by:',
array_merge(array_keys($options), ['<- Back'])
);
$questionHelper = new QuestionHelper();
Copy link
Contributor

Choose a reason for hiding this comment

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

The display for the setup renders as follows:

Configure Filter: Has Label
Select Label to filter by:
  [0] 1 Alpha
  [1] 3 Alpha
  [2] 4 Bravo
  [3] <- Back
 >

You can rely on the ChoiceQuestion to do the option numbering for you

*/
static public function getName()
{
return 'Has Label';
Copy link
Contributor

@mostertb mostertb Nov 10, 2019

Choose a reason for hiding this comment

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

If you choose to go implement the functionality in #2, then this should probably change to something like Label Name to differentiate it from the alternate functionality of matching a specific Label.

See \AppBundle\Helper\CardFilter\LabelColourCardFilter for reference

*/
public function getConfigDescription()
{
return 'Label: '.$this->getTargetLabel()->getName();
Copy link
Contributor

Choose a reason for hiding this comment

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

If you chose to go implement matching a specific label, then have a look at how the labels are rendered in \AppBundle\Command\CardFilterCommand::printFilteredCards()

This covers Labels without names and attempts to differentiate Labels by colour

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.

None yet

2 participants