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

refactor(ied/da-container,do-container): code improvements #464

Merged
merged 6 commits into from
Jan 10, 2022

Conversation

JakobVogelsang
Copy link
Collaborator

@Flurb as you did not like the toggle function I tried to refactor. What do you think.

I do get snapshot updates that I cannot interpret. Maybe you can have a look.

@JakobVogelsang JakobVogelsang requested a review from Flurb January 8, 2022 22:47
@Flurb
Copy link
Contributor

Flurb commented Jan 9, 2022

Way better! Using standard components instead of your own implementation is always better.
I couldn't find one, but I think I might have searched wrong, mwc-icon-button-toggle is perfect here.

Copy link
Contributor

@Flurb Flurb left a comment

Choose a reason for hiding this comment

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

Looks good, but during testing is see that the icon isn't changing when toggling.

Copy link
Contributor

@Flurb Flurb left a comment

Choose a reason for hiding this comment

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

Also, the same toggle principle can be added to the ln-container :)

@JakobVogelsang
Copy link
Collaborator Author

Also, the same toggle principle can be added to the ln-container :)

This was on purpose from my side (see #461). My opinion is that we need some other mechanism here. Do you agree? Do I miss something?

@JakobVogelsang
Copy link
Collaborator Author

Way better! Using standard components instead of your own implementation is always better. I couldn't find one, but I think I might have searched wrong, mwc-icon-button-toggle is perfect here.

This is bug of this component that only is present in debug mode. Just for you to compare, see the buttons in the log.

@Flurb
Copy link
Contributor

Flurb commented Jan 9, 2022

Way better! Using standard components instead of your own implementation is always better. I couldn't find one, but I think I might have searched wrong, mwc-icon-button-toggle is perfect here.

This is bug of this component that only is present in debug mode. Just for you to compare, see the buttons in the log.

Then it's fine, hope it will be fixed soon!

@Flurb
Copy link
Contributor

Flurb commented Jan 9, 2022

Also, the same toggle principle can be added to the ln-container :)

This was on purpose from my side (see #461). My opinion is that we need some other mechanism here. Do you agree? Do I miss something?

Why would this functionality be different for a LN container compared to DO/DA containers?

@JakobVogelsang
Copy link
Collaborator Author

JakobVogelsang commented Jan 10, 2022

Also, the same toggle principle can be added to the ln-container :)

This was on purpose from my side (see #461). My opinion is that we need some other mechanism here. Do you agree? Do I miss something?

Why would this functionality be different for a LN container compared to DO/DA containers?

Because rendering the data model for a complicated IED does take some seconds which in my opinion is not a good user experience. The hidden attribute is a performance boost, but as is, we still walk through every DO and DA there is in a data model of an IED. We don't need to do that. I think we should on first render only consider the data model up until the logical nodes and only on toggle button trigger should dive into the DOs and DAs. :)

Copy link
Contributor

@Flurb Flurb left a comment

Choose a reason for hiding this comment

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

I'm going to implement a solution for all three the containers, should not be a problem :)

@Flurb
Copy link
Contributor

Flurb commented Jan 10, 2022

@JakobVogelsang (or anyone else) please review :)

@JakobVogelsang JakobVogelsang requested a review from Flurb January 10, 2022 20:45
@JakobVogelsang
Copy link
Collaborator Author

LGTM @Flurb

@Flurb Flurb merged commit 081eac7 into add-da-container Jan 10, 2022
@Flurb Flurb deleted the minor-improvements-to-da-container branch January 10, 2022 20:46
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.

2 participants