-
-
Notifications
You must be signed in to change notification settings - Fork 150
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
Ordering front end redesign #4595
base: main
Are you sure you want to change the base?
Conversation
Add multiple tab specific endpoints
Rewrite and waffle the new UI changes Added a number of methods to fetch and/or store related and cited by data quickly Implemented new view opinion with waffles
Generally just override flags to avoid testing old view opinion page against the new ui changes.
Remove decorator for selenium tests unaffected And modify css to only affect scrolling on opinion page
Remove print statement and fix return for bot or scraping detection
Hide unwanted content during printing
Additionally, improves the printing process |
Additionally, addresses #4279 Moving page numbers to the right side of the sidebar |
update CSS and base.js to handle more edge case specific page numbering
Should address #1496 the strange intersection of footnotes and pagination in Anon/Tax import cases which appear to have competing and confusing changes. |
#4379 Adds support for ordered opinions and only displays either the combined opinion or the separated ordered opinions. |
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.
Looks great and it's lovely to see a number of other bugs getting squashed. A couple quick drive-by comments, then I'm afraid somebody else will need to do the full review.
-
The CSS is HUGE, and I'm afraid that if there is any extra code in there, we'll be carrying it around forever. I think we should put it in its own file so it's only on opinion pages (even though it'll be cached). On top of that, we should go through it as carefully as humanly possible to see if it can be optimized, documented, cleaned up, etc.
-
The JS file is also large and doesn't need to be on every page, so it should go in it's own file too.
That's it for me for the moment. Can't wait to see this live.
Move new js and css into own files
Was holding off on removing details but can re-add it if necessary
Also include the independent sections if the headmatter is separated out and the headmatter field is 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.
@flooie Here's my first round of comments on the code. Tomorrow, I’ll be diving into the CSS
, JavaScript
, and performance. So, expect more comments coming your way!
cl/tests/test_visualizations.py
Outdated
@@ -5,6 +5,7 @@ | |||
from django.contrib.auth.hashers import make_password | |||
from selenium.webdriver.common.by import By | |||
from timeout_decorator import timeout_decorator | |||
from waffle.testutils import override_flag |
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're not using this import, we should clean 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.
nice catch. thanks ill remove all of those
Co-authored-by: Eduardo Rosendo <[email protected]>
Co-authored-by: Eduardo Rosendo <[email protected]>
Co-authored-by: Eduardo Rosendo <[email protected]>
Refactor returned object in es_get_cited...with cache to re-use dataclass in system Add some improved doc strings
And add docstrings
:param request:The user request | ||
:return:Related Cluster Data | ||
""" | ||
cache = caches["db_cache"] |
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.
is there a reason to use database caching over Redis?
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 was attempting to be consistent with other elastic cached results
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 tend to do DB caching for larger objects, myself, but it's a fun fact that Django only evicts DB cache entries when you try to access them again later. If you cache something today, it expires, and you never visit it again, it'll take up storage forever. One day we should make a script for Django itself that cleans this up.
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.
As promised, here's the second round of comments. I've taken a closer look at the CSS, JavaScript, and overall performance.
context = { | ||
"tab": tab, | ||
"title": title, | ||
"caption": await cluster.acaption(), |
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 acaption
method consistently fires three database queries. Using prefetch_related
or select_related
won't help because it fetches the docket and court directly using the DB models. Here's the current implementation:
courtlistener/cl/search/models.py
Lines 2820 to 2822 in 5ac88cd
cluster = await OpinionCluster.objects.aget(pk=self.pk) | |
docket = await Docket.objects.aget(id=cluster.docket_id) | |
court = await Court.objects.aget(pk=docket.court_id) |
We can optimize this by using prefetched properties (if available) or triggering new queries only when needed. Here's a version that's working for me:
- cluster = await OpinionCluster.objects.aget(pk=self.pk)
- docket = await Docket.objects.aget(id=cluster.docket_id)
+ docket = await sync_to_async(lambda: self.docket)()
- court = await Court.objects.aget(pk=docket.court_id)**
+ court = await sync_to_async(lambda: docket.court)()
If we choose to implement the earlier suggestion, we can enhance the performance of render_opinion_view
by prefetching the related docket
and court
data.
def ordered_opinions(self): | ||
# Fetch all sub-opinions ordered by ordering_key | ||
sub_opinions = self.sub_opinions.all().order_by("ordering_key") | ||
|
||
# Check if there is more than one sub-opinion | ||
if sub_opinions.count() > 1: | ||
# Return only sub-opinions with an ordering key | ||
return sub_opinions.exclude(ordering_key__isnull=True) | ||
|
||
# If there's only one or no sub-opinions, return the main opinion | ||
return sub_opinions |
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're calling this function twice in the template, leading to two database queries. Let's use the cached_property
decorator to optimize and avoid redundant queries:
from django.utils.functional import cached_property
class OpinionCluster(AbstractDateTimeModel):
...
@cached_property
def ordered_opinions(self):
...
document.addEventListener('scroll', function () { | ||
let sections = document.querySelectorAll('.jump-link'); | ||
let links = document.querySelectorAll('.jump-links > a'); | ||
let currentSection = ''; | ||
|
||
// Determine which section is currently in view | ||
sections.forEach((section) => { | ||
let sectionTop = section.offsetTop; | ||
let sectionHeight = section.offsetHeight; | ||
if (window.scrollY >= sectionTop - sectionHeight / 3) { | ||
currentSection = section.getAttribute('id'); | ||
} | ||
}); | ||
|
||
// Remove the active class from all links and their parent elements | ||
links.forEach((link) => { | ||
link.classList.remove('active'); | ||
if (link.parentElement) { | ||
link.parentElement.classList.remove('active'); | ||
} | ||
}); | ||
|
||
// Add the active class to the link and its parent that corresponds to the current section | ||
links.forEach((link) => { | ||
if (link.getAttribute('href') === `#${currentSection}`) { | ||
link.classList.add('active'); | ||
if (link.parentElement) { | ||
link.parentElement.classList.add('active'); | ||
} | ||
} | ||
}); | ||
}); |
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 optimize this event listener by implementing a few tweaks. Here's a breakdown:
-
Reduce Iterations: Instead of looping through all nav links twice, we can initially query only the links with the active class. This significantly reduces the number of elements to process in the first loop.
-
Use Unique IDs: To streamline the process further, we can assign unique IDs to the target anchors. This allows us to directly target the specific element using
getElementById
, eliminating the need for a second loop and href attribute comparisons.
Here's my suggestion for this event listener:
document.addEventListener('scroll', function () { | |
let sections = document.querySelectorAll('.jump-link'); | |
let links = document.querySelectorAll('.jump-links > a'); | |
let currentSection = ''; | |
// Determine which section is currently in view | |
sections.forEach((section) => { | |
let sectionTop = section.offsetTop; | |
let sectionHeight = section.offsetHeight; | |
if (window.scrollY >= sectionTop - sectionHeight / 3) { | |
currentSection = section.getAttribute('id'); | |
} | |
}); | |
// Remove the active class from all links and their parent elements | |
links.forEach((link) => { | |
link.classList.remove('active'); | |
if (link.parentElement) { | |
link.parentElement.classList.remove('active'); | |
} | |
}); | |
// Add the active class to the link and its parent that corresponds to the current section | |
links.forEach((link) => { | |
if (link.getAttribute('href') === `#${currentSection}`) { | |
link.classList.add('active'); | |
if (link.parentElement) { | |
link.parentElement.classList.add('active'); | |
} | |
} | |
}); | |
}); | |
document.addEventListener('scroll', function () { | |
let sections = document.querySelectorAll('.jump-link'); | |
let currentSection = ''; | |
// Determine which section is currently in view | |
sections.forEach((section) => { | |
let sectionTop = section.offsetTop; | |
let sectionHeight = section.offsetHeight; | |
if (window.scrollY >= sectionTop - sectionHeight / 3) { | |
currentSection = section.getAttribute('id'); | |
} | |
}); | |
if (!currentSection) currentSection = 'top'; | |
// Remove the active class from links and their parent elements | |
let links = document.querySelectorAll('.jump-links > a.active'); | |
links.forEach((link) => { | |
link.classList.remove('active'); | |
if (link.parentElement) { | |
link.parentElement.classList.remove('active'); | |
} | |
}); | |
// Add the active class to the link and its parent that corresponds to the current section | |
let activeLink = document.getElementById(`nav_${currentSection}`); | |
if (!activeLink) return; | |
activeLink.classList.add('active'); | |
if (activeLink.parentElement) { | |
activeLink.parentElement.classList.add('active'); | |
} | |
}); |
Beyond the script adjustments, we'll need to add IDs to the opinions.html
template. To make this refactor easier to review, I've created a new branch. You can check the diff here:
Co-authored-by: Eduardo Rosendo <[email protected]>
Co-authored-by: Eduardo Rosendo <[email protected]>
Co-authored-by: Eduardo Rosendo <[email protected]>
Co-authored-by: Eduardo Rosendo <[email protected]>
UI Changes for Opinions Pages and associated view and utility changes.
PR adds
HTML
opinions.html
opinion-tabs.html
add-download-button.html
CSS and JS
base.js to modify footnotes and page numbers across various opinion sources.
override.css - to provide a new UI style for the front end and interactions.
py
Adds or updates new opinion views
for the Opinion, Summaries, Authorities, PDF/HTML document, Cited By and Related Cases
Overhauls the sidebar and orders opinions.
Improves the readability of footnotes and moves the three buttons to the top right for ease of access.