Skip to content
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

Virtualize Bounding Box List #7974

Merged
merged 9 commits into from
Aug 7, 2024
Merged

Virtualize Bounding Box List #7974

merged 9 commits into from
Aug 7, 2024

Conversation

dieknolle3333
Copy link
Contributor

@dieknolle3333 dieknolle3333 commented Aug 5, 2024

URL of deployed dev instance (used for testing):

Steps to test:

  • Open an annotation and go to bounding box tab.
  • Make sure you have at least 25 bounding boxes. The list should scroll quite some content.
  • UI wise, nothing should be different than before, test that. It should look and behave as before (layout, scrolling, etc)
  • Check that only the visible bounding boxes (plus some for padding) are loaded, e.g. by inspecting the html.

Issues:


(Please delete unneeded items, merge only when none are left open)

  • Updated changelog
  • Removed dev-only changes like prints and application.conf edits

@dieknolle3333 dieknolle3333 self-assigned this Aug 5, 2024
@dieknolle3333 dieknolle3333 marked this pull request as ready for review August 5, 2024 14:53
@philippotto
Copy link
Member

The code looks good and the UI too 👍 however, there is one thing: due to the "add bbox button", there is a native scrollbar (in addition to the virtualized but invisible one):

image

I think, you need to adapt the layout so that the div, that contains the table and the plus-button, is exactly as tall as there is available space. I see two options for that:
a) use a flexbox for that div (not sure how well this behaves with the autosizer). ah, I just see that the button is in a footer row. so, this option probably doesn't work.
b) when the auto-sizer provides a height of Y pixels, subtract the 38 pixels (the height of the button) from that height when passing it along to the table.

since the plus-button is part of the table, I'm not sure why this is happening in the first place. maybe the culprit is somewhere else. can you reproduce this?

@dieknolle3333
Copy link
Contributor Author

dieknolle3333 commented Aug 6, 2024

since the plus-button is part of the table, I'm not sure why this is happening in the first place. maybe the culprit is somewhere else. can you reproduce this?

Oh yes, I can reproduce this. I didnt notice that this was an additional scroll bar. But yeah, its weird that this happens!
Anyway, its fixed with your second option! So I'll just leave it at that I think. I didnt see any comment in the docs or any option to include the footer height.
edit: I didnt really find anything special. setting the scroll.y leads to the table content being that height, excluding the footer, but that was almost clear before. I will just leave it with the other fix.

Copy link
Member

@philippotto philippotto left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

works well 🎉

@dieknolle3333 dieknolle3333 enabled auto-merge (squash) August 7, 2024 08:57
@dieknolle3333 dieknolle3333 merged commit bf1b73c into master Aug 7, 2024
2 checks passed
@dieknolle3333 dieknolle3333 deleted the virtualize-bb branch August 7, 2024 09:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Virtualize the bounding box tab content
2 participants