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

Added option to select the Target in Link Field #16326

Merged
merged 10 commits into from
Jun 16, 2024
Merged

Added option to select the Target in Link Field #16326

merged 10 commits into from
Jun 16, 2024

Conversation

vengi83644
Copy link
Contributor

@vengi83644 vengi83644 commented Jun 15, 2024

Fixes #16244

image

image

@vengi83644
Copy link
Contributor Author

#16244

vengi83644 and others added 7 commits June 16, 2024 10:10
…eld.Edit.cshtml

Co-authored-by: Mike Alhayek <mike@crestapps.com>
…eldSettings.Edit.cshtml

Co-authored-by: Mike Alhayek <mike@crestapps.com>
…eldSettings.Edit.cshtml

Co-authored-by: Mike Alhayek <mike@crestapps.com>
…eld.cshtml

Co-authored-by: Mike Alhayek <mike@crestapps.com>
…eld.Edit.cshtml

Co-authored-by: Mike Alhayek <mike@crestapps.com>
…/LinkFieldIndexProvider.cs

Co-authored-by: Mike Alhayek <mike@crestapps.com>
@hishamco hishamco requested a review from MikeAlhayek June 16, 2024 11:34
…eld.Edit.cshtml

Co-authored-by: Hisham Bin Ateya <hishamco_2007@yahoo.com>
@hishamco
Copy link
Member

@MikeAlhayek waiting for your approval or I will merge tonight

@MikeAlhayek MikeAlhayek merged commit f4204c4 into OrchardCMS:main Jun 16, 2024
15 checks passed
@sebastienros
Copy link
Member

Don't we need to update the migration if there is a new field in the index table?
Do we really need to index link targets? Is there any business need to query links by their html target?

@vengi83644
Copy link
Contributor Author

We had a situation where we wanted to know how and where we used external links in our pages. One example page is this https://worldatwork.org/events

I missed the migrations. I will create a PR soon.

@sebastienros
Copy link
Member

I feel like this is mixing business and ui logic. I would assume these kind of links are specific to a very special kind of entity it's describing, and the fact this external link is opening in a new window is just based on this flag. For instance, a "Vendor" could be "External" and based on that the link to the vendor should use a "_blank" link.

This not only would feel right (ok this is a personal opinion) but this would not require this "niche" feature (indexing UI values). Otherwise this opens us to accepting everything in the index.

@sebastienros
Copy link
Member

@vengi83644 just to try to prove my point, where in your site, as a user, do I see the list of the things that are filtered by this new indexed value? Like is there a page of specific things that open in _blank, or a checkbox/filter for that?

@MikeAlhayek
Copy link
Member

@sebastienros I think we should have the Target as settings but there is no need for indexing it.

@vengi83644
Copy link
Contributor Author

@sebastienros @MikeAlhayek @Piedone We let our content authors get reports from the CMS for various use cases - examples: contents using certain widgets, certain content types with their title and description, contents of widgets used in different content types, a typical SEO report. Below is the screenshot of the page,
image

I am OK with removing the field from the index as it's not a use case for the larger audience. Thanks.

@Piedone
Copy link
Member

Piedone commented Jul 3, 2024

OK, thanks, we're removing it, then. Please follow Sebastien's above recommendations instead.

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.

[LinkField] Add option to select the Target - _blank, _self, _parent and _top
5 participants