-
Notifications
You must be signed in to change notification settings - Fork 51
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
Collection contact information #4091
Conversation
…ow view. Co-authored-by: Dananji Withana <dwithana@iu.edu> Co-authored-by: Phil Dinh <phuongdh@gmail.com>
@@ -67,7 +67,7 @@ const CollectionDetails = ({ content = '', email = '', website = '' }) => { | |||
{ website && | |||
<> | |||
<dt style={descriptionStyle}>Website:</dt> | |||
<dd><a href={website}>{website}</a></dd> | |||
<dd dangerouslySetInnerHTML={{ __html: website }}></dd> |
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.
@phuongdh @Dananji This is equivalent to innerHTML
and while it is called dangerously
because of the possibility of XSS I think it should be safe here since we're in control of the html. I thought it made more sense for the presenter to return a full a tag instead of the label and url separately. What do you two think?
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, sounds good to me 👍
@Dananji I know we talked about keeping the UI consistent with having the field labels present even when there isn't a value but I went back on that when seeing it in the collection landing page. Was that a bad idea? Maybe we can ask @joncameron tomorrow. |
When this gets merged and is on Spruce I'll take a look and we can tweak it. In this case I don't think it makes sense to have labels with no value. |
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.
👍
it { is_expected.to allow_value('collection@example.com').for(:contact_email) } | ||
it { is_expected.not_to allow_value('collection@').for(:contact_email) } | ||
it { is_expected.to allow_value('https://collection.example.com').for(:website_url) } | ||
it { is_expected.not_to allow_value('collection.example.com').for(:website_url) } |
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.
This may be a little a little too strict in practice as people aren't used to typing out the "https://" part, but I guess it makes for a safer validator.
property :contact_email, predicate: Avalon::RDFVocab::Collection.contact_email, multiple: false do |index| | ||
index.as :stored_sortable | ||
end | ||
property :website_label, predicate: Avalon::RDFVocab::Collection.website_label, multiple: false do |index| | ||
index.as :stored_sortable | ||
end | ||
property :website_url, predicate: Avalon::RDFVocab::Collection.website_url, multiple: false do |index| | ||
index.as :stored_sortable | ||
end |
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.
@jlhardes Are there standard RDF predicates that we should be using for these?
Resolves #3604