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 : hide checkbox in upload modal by default #22093

Merged
merged 14 commits into from
Jan 29, 2025

Conversation

mukulpadwal
Copy link
Contributor

What does it do?

This change addresses the unintended display of checkboxes in the upload modal by setting the isSelectable prop in the AssetCardBase component to false by default. Previously, isSelectable was set to true, which led to checkboxes being shown alongside each asset even though users could not select them.

To resolve this, I updated the default value of the isSelectable property in the AssetCardBase component to false. With this change, the checkbox only appears when isSelectable is explicitly set to true, allowing the upload modal to display assets without checkboxes by default.

This ensures that assets in the upload modal are listed solely for preview, not selection, resulting in a cleaner and more intuitive user experience.

Why is it needed?

The checkbox is currently shown for assets listed in the upload modal, despite the fact that these assets are not selectable. This can create confusion for users, as it implies that they can select or deselect assets in this context. By hiding the checkbox, we clarify the purpose of the modal and improve the user experience.

How to test it?

  1. Open the Media Library Upload Modal : Navigate to the Media Library and initiate the upload process by clicking on the "Add new assets" button.
  2. Add Assets for Upload : Select several assets from your file system to upload, so they appear in the upload modal's list of assets ready for upload.
  3. Verify Checkbox Visibility : Check that each asset listed in the upload modal appears without a checkbox. This confirms that the isSelectable property is now set to false, preventing unnecessary selection options in this context.

A demo video showing the expected behavior after this fix has also been uploaded with this pull request for reference.
FIX VIDEO

Related issue(s)/PR(s)

This PR addresses #22083 by hiding the checkbox in the upload modal where asset selection is not applicable.

Copy link

vercel bot commented Nov 7, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
contributor-docs ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jan 29, 2025 8:46am

@strapi-cla
Copy link

strapi-cla commented Nov 7, 2024

CLA assistant check
All committers have signed the CLA.

@oiorain oiorain added the community Changes and fixes created by community members label Nov 7, 2024
@hanpaine
Copy link
Contributor

hanpaine commented Nov 8, 2024

Hey @mukulpadwal thank you for another contribution 🔥 👏 we'll take a look asap

@lucasboilly lucasboilly linked an issue Nov 26, 2024 that may be closed by this pull request
@lucasboilly
Copy link
Contributor

@mukulpadwal all good on the design side, thanks!

@markkaylor markkaylor self-assigned this Dec 3, 2024
@markkaylor markkaylor self-requested a review December 3, 2024 14:17
markkaylor
markkaylor previously approved these changes Dec 3, 2024
Copy link
Contributor

@markkaylor markkaylor 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 to me, tested locally

@markkaylor markkaylor added pr: fix This PR is fixing a bug source: core:upload Source is core/upload package labels Dec 3, 2024
@markkaylor
Copy link
Contributor

markkaylor commented Dec 3, 2024

@mukulpadwal thanks for the contribution. Just a heads up your commits have these spaces that I think are making the commitlint unhappy

fix : message goes here
fix: message goes here

I will fix it when I squash and merge the PR

@markkaylor
Copy link
Contributor

markkaylor commented Dec 4, 2024

@mukulpadwal It looks like your changes had an impact on the FE unit tests. Could you please update them?

https://github.com/strapi/strapi/actions/runs/12141762514/job/33898382420?pr=22093

@markkaylor
Copy link
Contributor

Hi @mukulpadwal just a ping in case you didn't see my last comment, could you update the unit tests 🙏 ?

@mukulpadwal
Copy link
Contributor Author

Hi @markkaylor ,

Apologies for the delay in responding; I might have missed your last comment. I've updated the test cases as per your request. Please take a look, and let me know if there’s anything else you need or if any further changes are required.

Thanks! 😊

@markkaylor
Copy link
Contributor

@mukulpadwal there is still just one snapshot failing in a unit test 🤦‍♂️. You should just need to run this to fix it 🙏

yarn test:front packages/core/upload/admin/src/components/AssetGridList/tests/AssetGridList.test.tsx --updateSnapshot

@mukulpadwal
Copy link
Contributor Author

Hi, @markkaylor

Yes working on it. Will test thoroughly this time locally before pushing new changes. 🫡

Copy link
Contributor

@markkaylor markkaylor left a comment

Choose a reason for hiding this comment

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

tested again locally

@markkaylor markkaylor merged commit 64ae96a into strapi:develop Jan 29, 2025
50 of 51 checks passed
@markkaylor
Copy link
Contributor

@mukulpadwal thanks again for the contribution, see y'a on the next one ✌️

@innerdvations innerdvations modified the milestones: 5.9.1, 5.9.2 Feb 10, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
community Changes and fixes created by community members pr: fix This PR is fixing a bug source: core:upload Source is core/upload package
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Remove checkboxes on assets in the upload modal
7 participants