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

Initialize link dropdown description #894

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

JeffreyOfYOSPOS
Copy link

At present, when you select text and use the "link" command, a dropdown menu appears for the url itself and the description. However, the initial selected text is not propagated to the "description" box, which is confusing. Populate this box with any text that was selected before the url button was hit.

@github-actions github-actions bot added the lib label Aug 11, 2022
@samclarke
Copy link
Owner

Thanks for the PR!

This would be a much nicer UX for source mode but it would result in a different UX for WYSIWYG and source mode. I'm not sure if that's a good idea or not.

It's not really possible to show the selection in the description when in WYSIWYG mode as it could contain an image or something else that can't be represented in text.

Could possibly show a placeholder like [current selection] when in WYSIWYG mode although I'm not sure if that's better or not. Another possibility would be to remove the description field entirely and require changing the link text after inserting.

Copy link
Owner

@samclarke samclarke left a comment

Choose a reason for hiding this comment

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

Look good! Just missed one of the calls to _dropDown.

@@ -563,13 +563,14 @@ var defaultCmds = {

// START_COMMAND: Link
link: {
_dropDown: function (editor, caller, cb) {
_dropDown: function (editor, caller, selected, cb) {
Copy link
Owner

Choose a reason for hiding this comment

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

This causes an error in WYSIWYG mode as the call to _dropDown in exec: hasn't been updated to pass the selected parameter so cb is is being treated as selected and there is no cb passed.

@JeffreyOfYOSPOS
Copy link
Author

Thank you for the review! I'm sorry I did not catch that one issue - I will fix it.

I totally understand if you don't like the asymmetry with wysiwig mode. Something Awful is using SCEditor for the buttons primarily and I'm gonna keep thinking about how to make those work better, but I will do my best to test with wysiwig mode anything that I push upstream. I could see removing the description field in wysiwig mode exclusively, but, in my opinion, it is very important in source mode -it simultaneously simplifies the process of making a adding a link to a post, and also teaches new users the syntax for doing that directly.

A version where selecting stuff in wysiwig mode produced a placeholder would be neat for sure but also a much larger project than simply populating a text input with plaintext.

I would like it if we also used wysiwig mode some day but our set of accepted bbcode is large and weird and we'd basically end up writing a full bbcode parser in javascript. We have lower hanging fruit than that.

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

Successfully merging this pull request may close these issues.

None yet

2 participants