-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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
Added ContentPicker Validation For Required Fields #17470
base: main
Are you sure you want to change the base?
Conversation
Thank you for submitting your first pull request, awesome! 🚀 If you haven't already, please take a moment to review our contribution guide. This guide provides helpful information to ensure your contribution aligns with our standards. A core team member will review your pull request. |
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.
Regarding the style don't change this file instead change the one in the Styles
folder
OrchardCore/src/OrchardCore.Modules/OrchardCore.Resources/ResourceManagementOptionsConfiguration.cs
Lines 493 to 498 in 3de1e7f
manifest | |
.DefineStyle("vue-multiselect") | |
.SetUrl("~/OrchardCore.Resources/Styles/vue-multiselect.min.css", "~/OrchardCore.Resources/Styles/vue-multiselect.min.css") | |
.SetCdn("https://cdn.jsdelivr.net/npm/vue-multiselect@2.1.6/dist/vue-multiselect.min.css", "https://cdn.jsdelivr.net/npm/vue-multiselect@2.1.6/dist/vue-multiselect.min.css") | |
.SetCdnIntegrity("sha384-PPH/T7V86Z1+B4eMPef4FJXLD5fsTpObWoCoK3CiNtSX7aji+5qxpOCn1f2TDYAM", "sha384-PPH/T7V86Z1+B4eMPef4FJXLD5fsTpObWoCoK3CiNtSX7aji+5qxpOCn1f2TDYAM") | |
.SetVersion("2.1.6"); |
src/OrchardCore.Modules/OrchardCore.ContentFields/Views/ContentPickerField.Edit.cshtml
Outdated
Show resolved
Hide resolved
src/OrchardCore.Modules/OrchardCore.ContentFields/Views/ContentPickerField.Edit.cshtml
Outdated
Show resolved
Hide resolved
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.
Thank you!
Yep, what Hisham says. See https://docs.orchardcore.net/en/latest/guides/gulp-pipeline/. That's why https://github.com/OrchardCMS/OrchardCore/actions/runs/13251045412?pr=17470 fails.
When addressing review feedback, please adhere to the following:
- Please update your pull request according to feedback until it is approved by one of the core team members.
- Apply suggested changes directly so the reviewer doesn't have to eyeball the changes. These resolve themselves after applying them, and that's fine.
- Don't resolve other conversations so it's easier to track for the reviewer. Then, the reviewer will resolve them.
- Feel free to mark conversations that you addressed to keep track of them with an emoji or otherwise, just don't resolve them.
- Please keep conversations happening in line comments in those convos, otherwise, communication will be a mess. If you have trouble finding them, see this video.
- When you're done addressing all feedback of a review, click "Re-request review" in the top-right corner for each reviewer when you're ready for another round of review, so they know that you're done.
src/OrchardCore.Modules/OrchardCore.ContentFields/Views/ContentPickerField.Edit.cshtml
Outdated
Show resolved
Hide resolved
src/OrchardCore.Modules/OrchardCore.ContentFields/Views/ContentPickerField.Edit.cshtml
Outdated
Show resolved
Hide resolved
…tPickerField.Edit.cshtml Co-authored-by: Zoltán Lehóczky <zoltan.lehoczky@lombiq.com>
Unable to find the Style folder in OrchardCore.Resources -> Style Let me know what am I missing @hishamco ? |
It seems it's a bug that I need to fix after I check the input for the assets coming from Your thoughts @Piedone? |
No bug here, the asset file will output the styles & scripts in the |
You'll either need to add a new SCSS file to |
@Sandeep905 could you please share the style that has been added for this PR, looking into a minified version is hard, then we can check how we can go |
.Content-Picker-Required .multiselect__tags{ @hishamco here it is. |
So let us create a style file for it like what @Piedone suggested |
I didn't understand what you mean |
Are you creating the style file as suggested for the custom CSS I shared? |
I mean create a SCSS file with your changes |
@dotnet-policy-service agree |
@dotnet-policy-service agree company="Microsoft" |
@Sandeep905 maybe you can check this pr which applies the same fix but on taxonomy editors. |
It adds the exact CSS classes you have to use here too. So, good news, no need for new CSS! |
src/OrchardCore.Modules/OrchardCore.ContentFields/Views/ContentPickerField.Edit.cshtml
Outdated
Show resolved
Hide resolved
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.
There's a validation error now even if you select an item in the picker.
Also, when you're done addressing all feedback of a review, click "Re-request review" in the top-right corner for each reviewer when you're ready for another round of review, so they know that you're done.
Yes, @Piedone. It breaks when the class is added to Picker. I am trying to fix it. I tried to add a do not merge tag but could not do that. |
src/OrchardCore.Modules/OrchardCore.ContentFields/Views/ContentPickerField.Edit.cshtml
Outdated
Show resolved
Hide resolved
I see there's a new change after I approved, which is makes me worry was the previous class working properly or not?!! |
.has-validation-error .multiselect__tags { | ||
border: 1px solid red !important; | ||
background-image: url("data:image/svg+xml,%3csvg xmlns='http://www.w3.org/2000/svg' viewBox='0 0 12 12' width='12' height='12' fill='none' stroke='%23dc3545'%3e%3ccircle cx='6' cy='6' r='4.5'/%3e%3cpath stroke-linejoin='round' d='M5.8 3.6h.4L6 6.5z'/%3e%3ccircle cx='6' cy='8.2' r='.6' fill='%23dc3545' stroke='none'/%3e%3c/svg%3e"); | ||
background-repeat: no-repeat; | ||
background-position: right calc(0.375em + 0.1875rem) center; | ||
background-size: calc(0.75em + 0.375rem) calc(0.75em + 0.375rem) | ||
} | ||
|
||
.has-validation-error .multiselect__select { | ||
padding-right: calc(1.5em + 0.95rem) | ||
} |
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.
Don't repeat styling. Try to reuse the input-validation-error
class on an element. Better change the HTML structure of the editor to make it compatible with it than add a custom validation error CSS just for this case.
And as mentioned previously, if new CSS is needed, you'll need to run SCSS compilation with Gulp: https://docs.orchardcore.net/en/latest/guides/gulp-pipeline/. If something is Content Picker-specific, then it also needs to be added only the OrchardCore.ContentFields
module, and included from only this cshtml. If you search for "optionsEditor.css" in that project, you'll see an end-to-end example of this this can be done.
Fixes #17462
I tried to fix the above-linked Issue. @Piedone I am not sure if this approach is acceptable or not. If not let me know how I can fix the issue. This will be the first-ever open-source contribution from my end.
![image](https://private-user-images.githubusercontent.com/61585735/411648983-1521751f-d529-4b62-a32f-0358c5cec0c9.png?jwt=eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJpc3MiOiJnaXRodWIuY29tIiwiYXVkIjoicmF3LmdpdGh1YnVzZXJjb250ZW50LmNvbSIsImtleSI6ImtleTUiLCJleHAiOjE3Mzk2NjY2MzIsIm5iZiI6MTczOTY2NjMzMiwicGF0aCI6Ii82MTU4NTczNS80MTE2NDg5ODMtMTUyMTc1MWYtZDUyOS00YjYyLWEzMmYtMDM1OGM1Y2VjMGM5LnBuZz9YLUFtei1BbGdvcml0aG09QVdTNC1ITUFDLVNIQTI1NiZYLUFtei1DcmVkZW50aWFsPUFLSUFWQ09EWUxTQTUzUFFLNFpBJTJGMjAyNTAyMTYlMkZ1cy1lYXN0LTElMkZzMyUyRmF3czRfcmVxdWVzdCZYLUFtei1EYXRlPTIwMjUwMjE2VDAwMzg1MlomWC1BbXotRXhwaXJlcz0zMDAmWC1BbXotU2lnbmF0dXJlPWNjNjhlMDZlMjkxNTZjMjc2ZTc1NDZjN2M4ZGE0M2UwODFmYTYzMTliODcxNzFmOTM2ZWVkZmIxOWQzNjc2YjcmWC1BbXotU2lnbmVkSGVhZGVycz1ob3N0In0.5Gaa1N4QtLLtWewGu-jfITbQvJGpvxzHF_B1LPa5N3M)