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

SHS-5674: Help text for link fields is confusing #1702

Merged
merged 7 commits into from
Jan 13, 2025

Conversation

codechefmarc
Copy link
Collaborator

@codechefmarc codechefmarc commented Dec 19, 2024

READY FOR REVIEW

Summary

  • Creates dynamic help text for link fields using the default widget (not Linkit widgets) based on internal vs external requirements and regular vs menu usage.

Need Review By (Date)

2025-01-08

Urgency

low

Steps to Test

  1. Login to the site as an admin

Testing internal and external links

  1. Add a event
  2. Under Map Link, verify that the URL help text is: "Start typing the title of a piece of content to select it. You can also enter an internal path such as /about/contact-us or an external URL such as https://www.stanford.edu."

Screenshot 2024-12-18 at 5 08 58 PM

Testing external links only

  1. Add a course
  2. Under Course Link, verify that the help text is: "This must be an external URL such as https://www.stanford.edu."

Screenshot 2024-12-18 at 5 09 07 PM

Testing menu items and shortcuts

  1. Visit the main navigation list /admin/structure/menu/manage/main
  2. Edit an existing link or create a new link
  3. Verify that the Link help text is: "Start typing the title of a piece of content to select it. You can also enter an internal path such as /about/contact-us or an external URL such as https://www.stanford.edu. Enter to link to the front page. Enter to display a menu item that is only text without a link."

Screenshot 2024-12-18 at 5 16 22 PM

  1. Visit the shortcuts overview page /admin/config/user-interface/shortcut/manage/default/customize
  2. Edit an existing or create a new shortcut
  3. Verify the Path help text is: "This must be an internal path such as /about/contact-us You can also start typing the title of a piece of content to select it. Enter to link to the front page. Enter to display a menu item that is only text without a link."

Screenshot 2024-12-18 at 5 18 36 PM

Note: I did not find a field that used internal links only that wasn't the shortcut. So, to test, we can create a dummy field in a content type and set it to use internal links only. I did test this locally and it works fine:

Screenshot 2024-12-18 at 5 24 59 PM

PR Checklist


@ahughes3 ahughes3 temporarily deployed to Tugboat December 19, 2024 00:25 Destroyed
@ahughes3 ahughes3 temporarily deployed to Tugboat December 19, 2024 00:59 Destroyed
@ahughes3 ahughes3 temporarily deployed to Tugboat December 19, 2024 01:16 Destroyed
@codechefmarc codechefmarc self-assigned this Dec 19, 2024
@codechefmarc codechefmarc marked this pull request as ready for review December 19, 2024 01:26
Copy link
Collaborator

@cienvaras cienvaras left a comment

Choose a reason for hiding this comment

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

@codechefmarc Looks great, just a minor code improvement suggestion.

@ahughes3 ahughes3 temporarily deployed to Tugboat December 21, 2024 00:30 Destroyed
@codechefmarc
Copy link
Collaborator Author

Ready to re-test.

Copy link
Collaborator

@cienvaras cienvaras left a comment

Choose a reason for hiding this comment

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

@codechefmarc LGTM, thanks for the fixes.

@ahughes3 Ready for you to review.

@cienvaras cienvaras assigned ahughes3 and unassigned codechefmarc Jan 2, 2025
@cienvaras cienvaras requested a review from ahughes3 January 2, 2025 22:01
@ahughes3 ahughes3 temporarily deployed to Tugboat January 6, 2025 17:17 Destroyed
Base automatically changed from 11.6.1-release to develop January 8, 2025 16:39
Copy link
Collaborator

@ahughes3 ahughes3 left a comment

Choose a reason for hiding this comment

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

Approved!

@ahughes3 ahughes3 requested review from pookmish and sethprime January 8, 2025 20:22
@ahughes3 ahughes3 assigned pookmish and unassigned ahughes3 Jan 8, 2025
@pookmish
Copy link
Member

pookmish commented Jan 8, 2025

Would this be possible via this contrib module

@codechefmarc
Copy link
Collaborator Author

Would this be possible via this contrib module

@pookmish - This module would work for content entities, but not for menu items or shortcuts as the AC originally intended. Additionally, if we went this route, it would create a lot more config and each link field would have to be edited separately to make the change. If those link fields change what kinds of links they accept (internal, external, or both) then the help text would have to be updated. With the current approach, this is handled dynamically for any link field.

Let me know how you would like us to proceed. Thanks!

@ahughes3 ahughes3 temporarily deployed to Tugboat January 13, 2025 19:24 Destroyed
Copy link
Member

@pookmish pookmish left a comment

Choose a reason for hiding this comment

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

Ok, it's one of those tomato potato situations. I just wanted to bring up the option. I'm ok with this solution.

@pookmish pookmish changed the base branch from develop to 11.6.3-release January 13, 2025 20:18
@pookmish pookmish merged commit 8e364ca into 11.6.3-release Jan 13, 2025
17 checks passed
@pookmish pookmish deleted the SHS-5674--help-text branch January 13, 2025 20:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants