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

SC-206 Filters out used Channels #531

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

SC-206 Filters out used Channels #531

wants to merge 3 commits into from

Conversation

collins-self
Copy link
Contributor

Added a filterChannel function

Comment on lines 278 to 282
function filterChannels(chs: OpenXDA.Types.Channel[]) {
return chs.filter(ch =>
!(ch.Asset.length > 0)
);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Can this be wrapped with a useMemo instead of being a function? That way, we only run this function once any time props.Channels changes and not a bunch of times every rerender.

Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure that's true. I don't think we run this function on every rerender. It just get's defined as a new function on every re-render. that may be perfectly fine.
But the real question is why are we filtering through this twice?
ChannelSelector will filter it by whatever matches the Search state, so it will run through the list anyway. It would make sense to add this Filter there instead of filtering it here and in ChannelSelector

@clackner-gpa
Copy link
Member

Thinking this through, we may need/want to retain the ability to "Unselect" Channels already tied to this asset. otherwhise there is an issue in the workflow where someone accidentally assigns a channel, closes the modal and has no way of undoing that.
So the Filter should keep those Channels assigned to THIS asset.

clackner-gpa

This comment was marked as duplicate.

Copy link
Member

@clackner-gpa clackner-gpa left a comment

Choose a reason for hiding this comment

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

See above

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