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

Fix select issues #11869

Merged
merged 5 commits into from
Sep 10, 2024
Merged

Fix select issues #11869

merged 5 commits into from
Sep 10, 2024

Conversation

momesgin
Copy link
Member

@momesgin momesgin commented Sep 9, 2024

Summary

Fixes: Epic => #11743
Linked issues => #11752 #11753 #11754 #11755 #11756

Occurred changes and/or fixed issues

Technical notes summary

Areas or cases that should be tested

Areas which could experience regressions

Screenshot/Video

11752:

11752.mov

11753:

11753.mov

11754:

11754.mov

11755 & 11756:

11755-6.mov

Checklist

  • The PR is linked to an issue and the linked issue has a Milestone, or no issue is needed
  • The PR has a Milestone
  • The PR template has been filled out
  • The PR has been self reviewed
  • The PR has a reviewer assigned
  • The PR has automated tests or clear instructions for manual tests and the linked issue has appropriate QA labels, or tests are not needed
  • The PR has reviewed with UX and tested in light and dark mode, or there are no UX changes

@momesgin momesgin added this to the v2.10.0 milestone Sep 9, 2024
@momesgin momesgin self-assigned this Sep 9, 2024
addObject(this.authConfig.allowedPrincipalIds, id);
const currentIds = this.authConfig.allowedPrincipalIds;
// add new id and remove duplicates
const updatedIds = [...new Set([...currentIds, id])];
Copy link
Contributor

Choose a reason for hiding this comment

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

I like it, It's elegant. Does Set preserve order?

We do use lodash though so I think we want to use https://lodash.com/docs/3.10.1#uniq. Notice they also have the unique() alias in case you'd want to use that.

Copy link
Member Author

Choose a reason for hiding this comment

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

@codyrancher yes Set preserves order, but as you pointed out, I should've used lodash one, thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants