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

Introducing Action Button for Dropdown #56

Merged
merged 16 commits into from Jan 28, 2019
Merged

Introducing Action Button for Dropdown #56

merged 16 commits into from Jan 28, 2019

Conversation

ghost
Copy link

@ghost ghost commented Jan 22, 2019

This PR would make it possible to

  • add one or multiple "action buttons" within the dropdown list
  • perform an async request for this action (e.g. create new item)
  • use beforeInsert() as callback to get information which item has been selected
  • use showDenotationChar (default true) to switch off rendering or the denotation char if needed

Nothing should change for how quill-mention works at the moment. my changes "should" not break anything already working.

I have extended the demo with an example:

screenshot from 2019-01-22 20-45-12

See also #54

@ghost ghost closed this Jan 22, 2019
@ghost
Copy link
Author

ghost commented Jan 22, 2019

Had to fix an ugly typo ^^

@ghost ghost reopened this Jan 22, 2019
@ghost
Copy link
Author

ghost commented Jan 25, 2019

@MadSpindel Hey :) Just wanted to ask how the chances are that this gets accepted and released anytime soon. Don't want to stress but I need this feature quite soon. If this takes longer or gets rejected I'd just stick with my fork but I just wanted to ask first.

@MadSpindel
Copy link
Member

I am not sure if I understand the use case for this? Do you have any real life-examples of other sites using this technique?

@ghost
Copy link
Author

ghost commented Jan 26, 2019

@MadSpindel Of course. There are many.

If you scroll up on github and use the search to look for repositories. The search buttons

  • In this repository
  • All Github

would be our action-buttons, the existing repositories would be our items.

screenshot from 2019-01-26 10-23-27

The difference here is of course that this is not in the context of an editor - that's why we cannot solve some problems by just using different URLs.

But also besides my own use-case which is a real life example on its own (but similar to the ones below), one could think of the following scenarios:

  • The user writes an offer and mentions items they sell. If the user cannot find the item, he clicks the button and the item gets created with that particular name on the fly (optionally it opens a dialog where the name of the item is already set with the query text)
  • You write a book (or an article) and you want to keep track of where you mention people and things. Thus, whenever you mention a person or a thing, you either search for it in the editor, or create that entity "on the fly" - now you don't have to switch to the "Entity Management Page" in order to do so
  • User searches for places, the results come from your own database. However, the user cannot find the place he was looking for so there is a button "Your place isn't here?" which, if clicked, runs an expensive Google Maps API request and tries to find the place
  • The items in the list presented do exist, but some items need further actions before they can be added to the editor. E.g. all items need a description. If the user clicks an item without that property, he gets prompted to enter a description first because the item is not ready for inserting into the editor yet
  • User wants to track stuff he eats and enters the names of the meals he consumes. If that meal is not already in that list, it gets created on the fly
  • You are on stackoverflow and you have a question about quill-mention - However, this tag does not exist yet but there is a button in that list now which says "Create "quill-mention" tag"

and there would be one more thing which I think should be possible:

  • Dropdown shows usually only n items but with this there may now be a button now which allows the user to extend it to m > n

@MadSpindel
Copy link
Member

Thank you @silentsnooc. I will try to review this today or tomorrow 👍

@ghost
Copy link
Author

ghost commented Jan 26, 2019

@MadSpindel Awesome! :)

I am already using this in my project (from my fork of course) and so far I did not have any troubles. I hope it stays like that. ^^

Copy link
Member

@MadSpindel MadSpindel left a comment

Choose a reason for hiding this comment

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

I am missing documentation about beforeInsert() and showDenotationChar in the README.

@ghost
Copy link
Author

ghost commented Jan 26, 2019

I updated README.md - is it comprehensible?

@MadSpindel
Copy link
Member

@silentsnooc I am not sure if this is the right way. What I would like to see - I guess - is making renderList/renderItem more flexible (e.g. enable action buttons). Because I think we want something like a custom callback for each item in the list. Maybe something like "onSelect"?

@ghost
Copy link
Author

ghost commented Jan 27, 2019

@MadSpindel Hm, what do you have in mind? I don't see a straight forward way to make renderItem a candidate for doing beforeInserts job. We need two things: A way to prevent insertion and a way to insert an item later on (e.g. after a server request).

Could you elaborate what you have in mind?

@MadSpindel
Copy link
Member

Sure, will try :) I think selectItem() should call something like this.options.onSelect(item). Default should be to insert the item, but making it an option make it possible for you to do different things based on item type (like a action button). In a way, that is what you are trying to do with your code but maybe not as clear?

@ghost
Copy link
Author

ghost commented Jan 27, 2019

@MadSpindel Okay but how would we prevent the insertion of an item? And does onSelect provide a method for inserting an item afterwards?

@MadSpindel
Copy link
Member

Since the insert will be in the this.options.onSelect, we can prevent the insertion by overriding the logic in the onSelect with our own custom onSelect.

@ghost
Copy link
Author

ghost commented Jan 27, 2019

Do you mean something like this:

onSelect(item, insertItem) {
  if (item.hasOwnProperty('query') {
    // Make server call and then insertItem(item);
  } else {
    insertItem(item);
  }
},

And the default would be of course:

this.options = {
  // ..
  onSelect(item, insertItem) {
      insertItem(item);
  },
  // ..
}

@MadSpindel
Copy link
Member

Yes, but default should only be insertItem without any if (item.hasOwnProperty('query') {

@ghost
Copy link
Author

ghost commented Jan 27, 2019

Alright - I can make these changes if you want :)

@MadSpindel
Copy link
Member

Sure, or if you make a new PR, it doesn't matter to me 👍

@ghost
Copy link
Author

ghost commented Jan 27, 2019

I'd just make those changes and push to this PR if that's okay with you :)

@ghost
Copy link
Author

ghost commented Jan 27, 2019

For onSelect documentation:

Callback for a selected item. When overriding this method, insertItem should be used to insert item to the editor. This makes async requests possible.

Is that okay or would you use a different wording/explanation?

@MadSpindel
Copy link
Member

MadSpindel commented Jan 27, 2019

Sounds good to me 👍

@ghost
Copy link
Author

ghost commented Jan 27, 2019

@MadSpindel I pushed all changes of my last commit e98be50 :)

README.md Outdated Show resolved Hide resolved
src/quill.mention.css Outdated Show resolved Hide resolved
src/quill.mention.js Outdated Show resolved Hide resolved
src/quill.mention.js Outdated Show resolved Hide resolved
@ghost
Copy link
Author

ghost commented Jan 27, 2019

@MadSpindel I just noticed that the dropdown is transparent:

screenshot from 2019-01-27 15-37-01

has this always been the case or did I break something?

@ghost
Copy link
Author

ghost commented Jan 27, 2019

I applied the requested changes. Only thing missing would be the integration for a custom class for styling and last but not least a rebuild for dist/.

@ghost
Copy link
Author

ghost commented Jan 27, 2019

Alright, I made the changes regarding the style classes and updated the example in the index.html. The only thing left would be to update the dist/ files unless we want to adapt something else?

@MadSpindel
Copy link
Member

I can fix /dist and merge to master tomorrow 👍

@ghost
Copy link
Author

ghost commented Jan 27, 2019

@MadSpindel Awesome! Thanks for accepting this PR :)

@MadSpindel MadSpindel merged commit 2be1518 into quill-mention:master Jan 28, 2019
@MadSpindel
Copy link
Member

Thank you for excellent job! 💯

@nezlicodes
Copy link

@MadSpindel Hi, do you have a demo working example of this?

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.

None yet

2 participants