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(editors/binding): Allow filtering of subscribed/unsubscribed data in binding editors #1149

Merged
merged 21 commits into from
Mar 26, 2023

Conversation

danyill
Copy link
Collaborator

@danyill danyill commented Jan 30, 2023

Closes #1062

Draft until we agree if any of this is a good idea, and I will add some tests.

I'm not comfortable with the filter button being somewhat shaded when the section is selected, and the background colour darkens. I have tried to deal with it as best I can, but perhaps there is a better way.

It also doesn't entirely play nicely when "Not Subscribed" is selected and the user does subscriptions (the activated item becomes hidden). Maybe we don't need this option?

@danyill danyill changed the title feat(editors/binding) Allow filtering of subscribed/unsubscribed data in binding editors feat(editors/binding): Allow filtering of subscribed/unsubscribed data in binding editors Feb 9, 2023
Copy link
Collaborator

@JakobVogelsang JakobVogelsang left a comment

Choose a reason for hiding this comment

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

The code looks good. There is only one thing that I would want to be changed:

  • On my Mac when I select the first item the button has a different color compared to selecting the second choice.

Another one thing is debatable and I leave it up to you:

  • when no FCDA is viable also do not show the control block. With many control blocks in the file this effect of this filter is lost. This could be possible with simple CSS rule I feel

@JakobVogelsang
Copy link
Collaborator

Thank you @danyill for your wonderful contribution to that thing that we are all working on.

@danyill
Copy link
Collaborator Author

danyill commented Mar 3, 2023

  • On my Mac when I select the first item the button has a different color compared to selecting the second choice.

I don't see that - can you provide a screenshot?

Here is what I see:

image

  • when no FCDA is viable also do not show the control block. With many control blocks in the file this effect of this filter is lost. This could be possible with simple CSS rule I feel

Yes, good suggestion - let's do that.

@danyill
Copy link
Collaborator Author

danyill commented Mar 4, 2023

when no FCDA is viable also do not show the control block. With many control blocks in the file this effect of this filter is lost. This could be possible with simple CSS rule I feel

The CSS is kind of simple but for my weak mind it was not so simple...

I take a moment to pity my reviewer. 🙏

This table may help my reviewer! It helped me...

image

KS = Keep Subscribed (Show Subscribed)
KNS = Keep Not Subscribed (Show Not Subscribed)

OTOH although it seems complex to me it is blindingly fast which I like. ❤️‍🔥 🐎

@danyill
Copy link
Collaborator Author

danyill commented Mar 4, 2023

OK, after making the review changes I am much happier with the outcome.

I have implemented local storage so the filter selection isn't ephemeral.
Part of using a boolean attribute means I must default to false so the states must be inverted which can be confusing to think about.

I use accessors for holding the filter state so I can use other properties to create a unique identifier for local storage.

@danyill
Copy link
Collaborator Author

danyill commented Mar 4, 2023

Thank you for these review comments, the product is greatly improved. 🎉

This is ready for some more testing and review.

If we are happy, then I can add some brief integration tests for the four plugins where it is used.

I'll also do something similar to this in #1187 for the subscriber-oriented view.

@danyill danyill marked this pull request as ready for review March 4, 2023 21:00
@danyill
Copy link
Collaborator Author

danyill commented Mar 5, 2023

I have tried to add some initial tests, but they are not working - and so far I cannot see why. Could I ask you to take a look at these please?

@danyill danyill requested a review from JakobVogelsang March 5, 2023 06:29
@danyill
Copy link
Collaborator Author

danyill commented Mar 6, 2023

I have tried to add some initial tests, but they are not working - and so far I cannot see why. Could I ask you to take a look at these please?

I have figured out the reasons for this, finally...

@danyill danyill dismissed JakobVogelsang’s stale review March 7, 2023 08:58

Addressed in re-worked updates

@danyill
Copy link
Collaborator Author

danyill commented Mar 7, 2023

I have finally resolved the issues with tests and would be grateful for a review of this.
Sorry for the messy commit history. There were a few mis-steps made amid some confusion but eventually I straightened myself out 😉

I will be happy to take another review and make further improvements as required.

@danyill
Copy link
Collaborator Author

danyill commented Mar 9, 2023

I think Jakob is unavailable for some time. I'd be very pleased to receive a review on this so I can incorporate it into my subscriber view which will be similar.

I've nominated a few people and I would be happy to discuss/go through it if anyone is interested or available to look at this.

@danyill danyill requested review from juancho0202 and removed request for JakobVogelsang March 23, 2023 19:11
Copy link
Contributor

@juancho0202 juancho0202 left a comment

Choose a reason for hiding this comment

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

LGTM!

@juancho0202
Copy link
Contributor

hey @danyill please merge main back into your branch 😬

@danyill danyill requested a review from juancho0202 March 26, 2023 08:05
@danyill
Copy link
Collaborator Author

danyill commented Mar 26, 2023

Thank you for the assistance on this PR and the detailed review. 👏

@danyill danyill merged commit 874be45 into openscd:main Mar 26, 2023
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.

Allow filtering in later binding to "only configured" data
3 participants