-
Notifications
You must be signed in to change notification settings - Fork 0
FREYA-1549: Implement Data Highlights App #26
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
base: main
Are you sure you want to change the base?
Conversation
For future reference, so many changes in a single commit is too much, this doesn't facilitate the review process :) |
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.
Requesting changes mostly for required discussions
--color-pp-almost-black: #1a1c1a; | ||
} | ||
|
||
/* Custom components for data highlights */ |
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.
We should discuss if we need all these 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.
We can discuss, maybe app-specific or maybe we can reduce these components once we will have all major apps created.
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.
Yes, I mean among the components listed here, I think only topic badges will used in multiple pages/apps. Everything else is mostly specific to highlights (and maybe editorials). In that case I prefer to not have components and just use in-line styling using tailwind.
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.
Agreed, I would keep everything inline for now, we are not in the stage where we can really think about how to factor the styles and optimise the utility classes, so let's not bother and keep this for later when things will be more stable :)
For reference: https://tailwindcss.com/docs/styling-with-utility-classes#managing-duplication
{{ topic.name }} | ||
</a> | ||
</span> | ||
{% empty %} |
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.
do we wanna do it this way or not have "topics" at all if no topics assigned ? I don't have a preference, just making this comment for a discussion.
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 implemented it this way because i think it helps editors in content management.
Moreover, it will provide consistency for all highlights.
But if you guys prefer to remove this if no topic assigned, I am okay with that also.
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 don’t have any preference, I am fine with either. Like I mentioned, made just so we discussed this with the whole team :)
</span> | ||
{% endfor %} | ||
</div> | ||
|
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.
would it be useful to have the "share this" html code in core/templates/share_this.html
and use {% include "share_this.html" %}
in required pages ?
Just another comment for discussion, we of course have to tweak the code a little if we gonna implement it :)
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 agree with your suggestion. I am just not sure where else we are using these sharing buttons? if we are only using in highlights then maybe keeping it here is more appropriate.
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 totally agree, currently only highlights and editorial have this I guess. If its gonna be the case with the new portal then maybe it’s not worth abstracting it. We can confirm this with Liane in the discussion 👍
{% for highlight in highlights %} | ||
<div class="data-highlight" | ||
data-search-text="{{ highlight.title|lower }} | ||
{{ highlight.summary|lower }} |
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.
Not sure if we should include summary in the search. Since the search is meant for "keywords" search I think all relevant keywords would be covered in title and search tags.
We can discuss it :)
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.
You're right that title and topics cover the main keywords.
However, I included summary because researchers often search by methodologies, concepts-focused (e.g., "sequencing", "viral evolution", "phylogenetic analysis") or technical terms that appear in summaries but not titles/topics.
As summary text is not that long, this comprehensive search helps users discover relevant content by research techniques.
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.
Yes, but those words will not be in the title or added tags ? that's what I wanna know :)
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.
No, these words are not in tags, topics, or title. These kinds of words usually goes into abstract or summary.
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.
Tags are something added manually in our case and usually they are like what you have mentioned in the example. I am not an expert in this, but discus this with Laine. I am just concerned about random false hits from other regular words. Thats also true for the title, but given the size of the title (compared to summary) it is very minimal.
Might be I am just overthinking, just confirm it with Liane and I am fine with her decision.
# Get active highlights created before this one, ordered by creation date | ||
older_highlights = DataHighlight.objects.filter( | ||
is_active=True, | ||
created_at__lt=self.created_at |
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.
Any particular reason not to include newer data highlights than the current one ?
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.
There is a debate either we chose Chronological or Similarity-Based approach.
Its a matter of choice.
I think for single detail highlight, researcher would like to know its progression and background which will be technically old highlights (i tried to looked into current highlights on the portal and realised that this is the same approach of Chronological order implemented).
As newer highlights will always be listed in order on the listing.
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.
The current similarity/related is based on the mentioned keywords and not the content or the study itself. In that case I don't how chronological order helps.
For example, if I am an user and I am reading an article on some "X" technology or method, I would be interested on similar articles (i.e. same "X" tech or method) irrespective of whether it is older or newer than the current article I am reading. I would actually be interested in newer similar articles actually 😄
Maybe that's just me, we can discuss this with the team and see what other feel.
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.
yes, we can discuss.
I am open for both approaches. I am having current approach as I think current Pathogen portal also showing older highlights only.
return [] | ||
return [tag.strip().lower() for tag in self.tags.split(',') if tag.strip()] | ||
|
||
def get_related_highlights(self, limit: int = 4, threshold: float = 0.1) -> List['DataHighlight']: |
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.
Point to discuss, could this be potential re-usable stuff ?
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.
Not sure. Similar to share buttons only editorials might need similar functionality.
We can discuss.
ddc4ffb
to
db00e0e
Compare
<!-- Main Content --> | ||
{% if highlight.content %} | ||
<div class="prose prose-lg max-w-none text-pp-dark-grey mb-8"> | ||
{% if highlight.rendered_content %} |
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 forgot to ask about this, why have thisif
block ? Will there be a scenario where there is no rendered_content
? 🤔
<!-- Announcement (if present) --> | ||
{% if highlight.announcement %} | ||
<div class="announcement"> | ||
{% if highlight.rendered_announcement %} |
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.
Same question here, will there be a scenario where there is no rendered_anouncement ?
📋 Summary
This PR implements a comprehensive Data Highlights app, providing a complete content management system for showcasing research findings and scientific discoveries.
🛠️ Changes Made
pages/highlights
app with complete model, views, admin, and templates🔍 Notes for Reviewers
✅ Checklist
FREYA-XXXX: Clear and short description
🔗 Jira Issue
Closes: FREYA-1549