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

changing the geplandOpenbaar of an agendapoint affects the openbaar of a behandeling #273

Merged
merged 2 commits into from
Jun 17, 2022

Conversation

lagartoverde
Copy link
Contributor

@lagartoverde lagartoverde commented May 4, 2022

@lagartoverde lagartoverde requested review from nvdk and abeforgit May 4, 2022 13:06
@nvdk nvdk requested review from benjay10 May 5, 2022 14:35
Copy link
Contributor

@benjay10 benjay10 left a comment

Choose a reason for hiding this comment

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

I don't understand what the did-update modifier is trying to achieve. I could not trigger it. Can someone explain?

Also, can the modifier be put on the top level component in this file? The effect should be the same, but that would make this PR much cleaner, by just adding 1 single line in this file.

@nvdk
Copy link
Member

nvdk commented May 16, 2022

I don't understand what the did-update modifier is trying to achieve. I could not trigger it. Can someone explain?

Also, can the modifier be put on the top level component in this file? The effect should be the same, but that would make this PR much cleaner, by just adding 1 single line in this file.

I'm not sure if it's needed here. The did-update notifier is typically useful if you have an argument passed into a component and want to trigger some function if the the value of the argument is updated. it can't be added to a component because it will be considered an attribute that's passed down into the component. However it could be added on another element, I don't think a wrapping div is required here and I also dislike the addition.

@lagartoverde Can you elaborate why the did-update modifier is needed here?

@lagartoverde
Copy link
Contributor Author

We ran into a bug in a previous support item, where the component was failing because the constructor was not being called in the production build as the elements were reused, that's why I added it here, so it would ran every time the argument was changed and not only on the construction of the element.
Made it safer and wasn't really an inconvenience but not sure if the same would happen on here

oldGeplandOpenbaar;
constructor() {
super(...arguments);
this.oldGeplandOpenbaar = this.args.itemToEdit.geplandOpenbaar;
Copy link
Member

Choose a reason for hiding this comment

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

why are you not just using a getter?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because I want to get the original value when the component was created not the actual value of the variable

@lagartoverde
Copy link
Contributor Author

I removed the did-update modifier as you didn't like it

@lagartoverde lagartoverde requested review from abeforgit and removed request for nvdk June 16, 2022 16:30
@nvdk nvdk force-pushed the feature/edit-planned-public-affects-public branch from fb3445a to f3f73f0 Compare June 17, 2022 09:53
@nvdk nvdk added the enhancement New feature or request label Jun 17, 2022
@nvdk nvdk merged commit 3dd6c1a into development Jun 17, 2022
@nvdk nvdk deleted the feature/edit-planned-public-affects-public branch June 17, 2022 09:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants