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

Search/filter command by pattern, function name, and effect type #21

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

Conversation

AlbertLucianto
Copy link

@AlbertLucianto AlbertLucianto commented Jan 6, 2018

alt text
alt text

type,
name: description.pattern
|| description.channel
|| description.action
Copy link
Member

Choose a reason for hiding this comment

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

sould be description.action.name ?

Copy link
Member

Choose a reason for hiding this comment

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

sorry description.action.type

Copy link
Author

Choose a reason for hiding this comment

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

Thanks for pointing it out. But, I wasn't sure with what are needed to be checked by the filter, because some of them have payload. So for now, I just made it as simple as possible. Do you think the filter better check for the payload including the keys and values as well?

rootEffectIds={this.props.rootEffectIds}
filter={this.state.filter}
setFilterOptions={this.setFilterOptions}
/>
Copy link
Member

Choose a reason for hiding this comment

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

There is also another EffectView deep inside ActionView/Reactions/EffectView which shows a similar effect tree but only for those sagas that reacted to the selected action. Right now when selecting an action the app crashes because the deeply nested EffectView receive a null filter. So we should forward filter options from ActionView down to it

Copy link
Author

Choose a reason for hiding this comment

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

Thanks for the review! I will fix it in the next PR.

@abettadapur
Copy link
Collaborator

@AlbertLucianto Were you interested in completing this PR?

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