-
Notifications
You must be signed in to change notification settings - Fork 361
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
upcoming: [DI-20934] - Configurable Max limit on resource selection component #11252
base: develop
Are you sure you want to change the base?
Conversation
Coverage Report: ✅ |
Cloud Manager UI test results🎉 445 passing tests on test run #5 ↗︎
|
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.
Verified:
- Any selections beyond the max limit are disabled
- The Select All option is only present when the number of resource options is less than the limit
- Test coverage is present and passing
Thanks @ankitaakamai, approving pending a couple of comments I left are addressed. I also called out one question about buggy UI behavior to see if the team is aware of it.
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.
While reviewing, I noticed some unexpected behavior here: after making a selection and closing the resource select, opening it back up displays a flash of content for the options. This appears to be happening with any number of selections (not just the max) and is happening on the develop
branch, so preexisting to this PR. Is this a bug that's already known and there is a ticket to address?
Screen.Recording.2024-11-13.at.9.27.28.AM.mov
"@linode/manager": Upcoming Features | ||
--- | ||
|
||
Configurable Max limit on resource selection component ([#11252](https://github.com/linode/manager/pull/11252)) |
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.
Configurable Max limit on resource selection component ([#11252](https://github.com/linode/manager/pull/11252)) | |
Configurable max limit on CloudPulse resource selection component ([#11252](https://github.com/linode/manager/pull/11252)) |
Could we mention CloudPulse here? Once this changeset becomes part of a full list of changes in a changelog, it's not always clear at a glance what product was impacted.
/> | ||
); | ||
|
||
fireEvent.click(screen.getByRole('button', { name: 'Open' })); |
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.
Please use userEvent
for the reasons described here in our developer docs. This was a recent update we made to update our best practices, so let's not rely on fireEvent
for new test coverage.
We recommend using userEvent rather than fireEvent where possible. This is a React Testing Library best practice, because userEvent more accurately simulates user interactions in a browser and makes the test more reliable in catching unintended event handler behavior.
expect(screen.getByText('Select up to 10 Resources')).toBeInTheDocument(); | ||
|
||
for (let i = 14; i <= 23; i++) { | ||
fireEvent.click(screen.getByRole('option', { name: `linode-${i}` })); |
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.
Same comment as above.
Description 📝
Introduction of configurable Max limit on resource selection component and other enhancements.
Changes 🔄
limit
label
' is introduced under resource selection autocomplete.Target release date 🗓️
12-12-2024
Preview 📷
How to test 🧪
Verification steps
As an Author, I have considered 🤔
As an Author, before moving this PR from Draft to Open, I confirmed ✅