-
Notifications
You must be signed in to change notification settings - Fork 4
Conversation
Here is a little demo with 3 reference providers:
The goal would be to open this "link picker" in Text and in the RichText component (that will be used by Talk one day). vokoscreenNG-2022-12-20_17-44-27.mp4 |
@nimishavijay @jancborchardt Do you like it? 😁 Any obvious visible design mistake? |
} | ||
|
||
this.renderResult = renderCustomPickerElement(this.$refs.customElement, { providerId: this.provider.id, accessible: false }) | ||
if (this.renderResult.object.$on) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if (this.renderResult.object.$on) { | |
if (this.renderResult?.object?.$on) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe also check if object is a vue instance here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very cool! :) Some design feedback:
- Would the dropdown element on the right not open in place of where you type
#
(or/
) like in Notion, instead of off to the right? - The "GitHub code permalink" option is a bit strange since you can just put in any link and it will just be put in the text – you could just type it in the text itself or paste it. Is there a benefit doing it this way? Otherwise I’d say let’s remove the entry?
- The standard from Notion seems to be the
/
character to invoke these kinds of pickers. Could we switch to that? (We can also support both so people familiar with Notion and GitHub are familiar.) - The Giphy gif picker could use a nice emptycontent view. In messaging apps that is often just popular Gifs, Giphy’s API probaby has that?
Notes from the design call we had earlier today with @nimishavijay
|
@julien-nc added a point to my review:
|
@jancborchardt Thanks for the review. Triggering the link picker with The overall appearance has changed since the screencast. Choosing the link provider can now be done like choosing an emoji. |
23833fb
to
101becc
Compare
link.picker.mp4Current state:
There are 3 ways to open the link picker:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wow, really amazing! :) I think it would be best to merge it so we can see it in action in different environments like Text, Talk, comments etc.
Only detail note: When writing "GitHub issues, pull reuquests […]", the "p" of pull requests is capital, but should be lowercase. :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just minor comments, otherwise good to go 🚀
loading: false, | ||
reference: null, | ||
abortController: null, | ||
// TODO translate? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's have this as a follow up task so we can get a first release out.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok and let's keep the comments if you don't mind.
@juliushaertl Let's wait a bit before merging this. There is still one thing left to do from the design review: get rid of the "raw link" providers and systematically add a "Any link" one as the last suggestion that opens the raw link input (with dynamic preview). Do you agree with this idea? |
Sounds good |
Signed-off-by: Julien Veyssier <julien-nc@posteo.net>
…gn-icons Signed-off-by: Julien Veyssier <julien-nc@posteo.net>
8858cae
to
2a4fbb2
Compare
@juliushaertl Rebased on master, added JS debug log for providers without related search providers or without custom picker component. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 👍
refs nextcloud/server#31667
requires nextcloud/server#35557
The reference picker gets the reference provider list via an initial state provided by the server. Whatever page using the reference picker should have dispatched the
RenderReferenceEvent
to trigger the production of the initial state.Each reference provider can declare a list of supported unified search providers. Those search provider will be used if no custom link picker is provided. The search UI is pretty similar to the core unified search: small thumbnail, a title and a sub line.
If the search UI does not fit the needs of the reference provider, it is possible to register a custom component to let users choose an element to get a link in the end. This component must emit the
submit
event with the link as event data. It an also emit thecancel
event to go back to the reference provider picker.To test this PR, one can:
reference-discovery
branch of the integration_github app (don't forget to link with@nextcloud/vue-richtext
while having theenh/noid/reference-picker
branch checked out)Remaining questions/issues/todos:
RichText
component (when typing a special character and/or when receiving an event via the NC event bus and/or via a component method call)