-
Notifications
You must be signed in to change notification settings - Fork 328
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
New command: viva engage community set. Closes #6279 #6283
New command: viva engage community set. Closes #6279 #6283
Conversation
Thanks, we'll try to review it ASAP! |
206e13a
to
48ce8ca
Compare
de78bb5
to
539e643
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @MartinM85, there are a few PRs open today related to viva engage community
. In one of those, @martinlingstuyl suggested some changes to the util to improve performance and make it more dynamic. Ref: #6388 (comment)
It might be worth considering those changes here too since you're working with the same util in a similar context.
@Jwaegebaert ok, I will update the util based on |
Great! I'm not entirely sure this is the best approach yet, so it could be subjected to change 🤞 |
@MartinM85 did you already had a chance to update the utils? I am not 100% sure if this is ready for a recheck? |
539e643
to
756450f
Compare
Hi @Adam-it, I was waiting to avoid update vivaEngage utils multiple times. Update right now, so it's ready for review |
8d94a32
to
d2c8853
Compare
Hi @Adam-it, do you plan to review this PR in the coming days/weeks? In the meantime, other commands related to the viva engage community are merged, so I'm just rebasing (this and #6286). It seems to me better to wait until the other commands with higher priority are merged. Now it's useless effort for me. |
TBH I was going to to that last time but I didn't manage. Today we started internal code freeze till upcoming major release which will be at the end of this month and during this time we only may merge pr-major pr-priority and docs PRs. This means this PR may proceed no sooner than at the beginning of November |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perfect 👏👏👏
ready to merge 🚀 |
Merged manually. Thank you for your awesome work! You Rock !🤩 |
Closes #6279