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

Consolidate admin required fields #15545

Merged
merged 6 commits into from
Mar 22, 2024

Conversation

xtomas
Copy link
Contributor

@xtomas xtomas commented Mar 19, 2024

  • add star char for all required fields
  • add validation message for required input field like in TitlePart
  • consolidate color for start and validation error messages
  • consolidate error message texts
  • remove client validation from inputs because of inconsistent form behaviour dependent on used browser, selected locale, ...

#15434

New test content item form with required fields

image

And the same content item after Publish

image

+ add star char for all required fields
+ add validation message for required input field like in TitlePart
* consolidate color for start and validation error messages
* consolidate error message texts
- remove client validation from inputs because of inconsistent form behaviour dependent on used browser, selected locale, ...

OrchardCMS#15434
@xtomas
Copy link
Contributor Author

xtomas commented Mar 19, 2024

@dotnet-policy-service agree

@hishamco
Copy link
Member

@all-contributors please add @xtomas for code.

Copy link
Contributor

@hishamco

I've put up a pull request to add @xtomas! 🎉

@xtomas xtomas requested a review from agriffard as a code owner March 22, 2024 01:36
@@ -6,11 +6,12 @@

<script asp-name="jQuery-ui-i18n" at="Foot"></script>
<div class="@Orchard.GetFieldWrapperClasses(Model.PartFieldDefinition)" id="@Html.IdFor(x => x.Value)_FieldWrapper">
Copy link
Contributor

@Skrypt Skrypt Mar 22, 2024

Choose a reason for hiding this comment

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

Have you tried with asp-validation-class-for="Value" on this div to set the entire "field" to be set has having a validation error?

Copy link
Contributor

Choose a reason for hiding this comment

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

Example:

image

@hishamco
Copy link
Member

@Skrypt do you have anything else to add or shall we merge this?

@Skrypt
Copy link
Contributor

Skrypt commented Mar 22, 2024

It's approved; you can merge but I'm just asking if we should not also add the asp-validation-class-for too then to make this consistent everywhere.

@hishamco
Copy link
Member

What's the wrong with asp-validation?

@hishamco hishamco merged commit bf828ea into OrchardCMS:main Mar 22, 2024
4 checks passed
@Skrypt
Copy link
Contributor

Skrypt commented Mar 22, 2024

It is missing everywhere for these fields. But we use it in ActivityDisplayDrivers editors for example.

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

Successfully merging this pull request may close these issues.

4 participants