-
Notifications
You must be signed in to change notification settings - Fork 97
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
Related resources #219
Related resources #219
Conversation
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.
Still reviewing, just a couple questions so far...
"status", | ||
"owners", | ||
], | ||
// TODO: Confirm this with a fresh index |
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.
Did you mean for this to stay in? I'm not really sure what it's in reference to.
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.
I think we can remove it but let's make sure to test this on staging.
I have Algolia index problems that prevent me from trying this locally.
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.
Consider the resources related (just some non-blocking questions/nits).
this.relatedDocuments = cachedDocuments; | ||
|
||
this.flashMessages.add({ | ||
title: "Unable to save", |
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.
Nit: might be somewhat helpful for the user to differentiate between another save (doc?) action.
title: "Unable to save", | |
title: "Unable to save related resources", |
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.
I kept this generic so it could also apply to non "Related resources"-labeled fields, e.g., "RFC." I've changed it to "Unable to save resource" which might split the difference? What do you think
const getLinkIconHelper = helper<GetLinkIconSignature>(([url]) => { | ||
if (url) { | ||
const urlParts = url.split("/"); | ||
let domain = urlParts[2]; |
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.
Should we have more URL validation at this level? What kind of UX is it if we fail the assert
below?
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.
Yeah good call. I've changed it from assert
to if...
so if the string doesn't match our formatting, it'll still return a generic icon.
Adds a
RelatedResources
component, allowing editors to attach first- and third-party links to their document.