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

Refactor link target inputs to dropdowns #17536

Closed
wants to merge 2 commits into from

Conversation

rwxzig
Copy link

@rwxzig rwxzig commented Feb 27, 2025

Replaced <input> elements with <select> dropdowns for the "Target" and "DefaultTarget" fields in multiple Razor view files. This change allows users to choose from predefined options (_self, _blank, _parent, _top) instead of manual input. Removed unnecessary <datalist> elements as part of this update.

Replaced `<input>` elements with `<select>` dropdowns for the "Target" and "DefaultTarget" fields in multiple Razor view files. This change allows users to choose from predefined options (`_self`, `_blank`, `_parent`, `_top`) instead of manual input. Removed unnecessary `<datalist>` elements as part of this update.
Copy link
Contributor

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.

@rwxzig
Copy link
Author

rwxzig commented Feb 27, 2025

@rwxzig please read the following Contributor License Agreement(CLA). If you agree with the CLA, please reply with the following information.

@dotnet-policy-service agree [company="{your company}"]

Options:

  • (default - no company specified) I have sole ownership of intellectual property rights to my Submissions and I am not making Submissions in the course of work for my employer.
@dotnet-policy-service agree
  • (when company given) I am making Submissions in the course of work for my employer (or my employer has intellectual property rights in my Submissions by contract or applicable law). I have permission from my employer to make Submissions and enter into this Agreement on behalf of my employer. By signing below, the defined term “You” includes me and my employer.
@dotnet-policy-service agree company="Microsoft"

Contributor License Agreement

@dotnet-policy-service agree

@hishamco
Copy link
Member

Please accept the CLA

@@ -66,13 +66,12 @@
<div class="mb-3">
<div class="w-md-75 w-xl-50">
<label asp-for="Stereotype">@T["Stereotype"]</label>
<input asp-for="Stereotype" list="stereotypeOptions" type="text" class="form-control">
<datalist id="stereotypeOptions">
<select asp-for="Stereotype" id="stereotypeOptions" class="form-select">
Copy link
Member

Choose a reason for hiding this comment

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

Please revert this change because stereotypes could be editable

Copy link
Author

Choose a reason for hiding this comment

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

I reverted what you asked for, but it still looks like it has been edited because it messed up the spacing.
But just I was thinking should the stereotypes be editable though? What are your thoughts @Piedone?

Copy link
Member

Choose a reason for hiding this comment

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

I might be wrong here, so I will wait for @MikeAlhayek

@Skrypt
Copy link
Contributor

Skrypt commented Feb 27, 2025

I think that the idea is that these options are only suggestions. They are not mandatory for these fields, this is why it is a datalist. Is there a reason to make them use asp-for over the design intent that seems to have been used?

The stereotype should be suggested values.

image

The HtmlMenuItemPart too

image

Skrypt
Skrypt previously requested changes Feb 27, 2025
Copy link
Contributor

@Skrypt Skrypt left a comment

Choose a reason for hiding this comment

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

I do not approve this PR. These value should stay non required.

@hishamco
Copy link
Member

The target values should be list to avoid inappropriate user data

@Skrypt
Copy link
Contributor

Skrypt commented Feb 27, 2025

Ok then, approved but not for Stereotype.

@Skrypt Skrypt dismissed their stale review February 27, 2025 02:55

approved

Skrypt
Skrypt previously approved these changes Feb 27, 2025
@MikeAlhayek
Copy link
Member

Select menu was done here by design. This way if a user wants to specify a custom value they can

Checkout the conversation here #15636 (comment)

@hishamco
Copy link
Member

Ok then, approved but not for Stereotype.

Totally agree, I already wrote a comment about stereotypes

@MikeAlhayek
Copy link
Member

@Skrypt @hishamco I don't think we should proceed with this. Look at my previous comment

@Skrypt
Copy link
Contributor

Skrypt commented Feb 27, 2025

Yeah, but for a <a target="customstring" ></a> I'm not sure if it makes sense.

https://developer.mozilla.org/en-US/docs/Web/HTML/Element/a#target

@Skrypt
Copy link
Contributor

Skrypt commented Feb 27, 2025

Ok nevermind it seems valid to use something like target="framename".

@Skrypt Skrypt dismissed their stale review February 27, 2025 04:13

disaprove as we can use custom values for all of these fields

@rwxzig rwxzig closed this Feb 27, 2025
@rwxzig rwxzig deleted the replace-datalist-with-select branch February 27, 2025 10:08
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