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

Fix height of container for creating new annotations #638

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

jduehring
Copy link
Contributor

This PR should fix #630 .

For some reason the default height for the container containing the labels was set to 100vh. I set this to auto so that it won't use the whole screen height.

I also changed the value for overflow-y to auto so that the scrollbar for labels is only visible, when neccessary.

@dagraf
Copy link
Collaborator

dagraf commented Dec 3, 2024

It now works as expected. Thx for the fix!

@JulianKniephoff JulianKniephoff self-assigned this Dec 5, 2024
@JulianKniephoff JulianKniephoff self-requested a review December 5, 2024 11:59
Copy link
Member

@JulianKniephoff JulianKniephoff left a comment

Choose a reason for hiding this comment

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

The culprit here was 61478c9.

However, I see no difference between what this PR does, and not applying this class at all, in which case we could get rid of it, too, since it isn't used anywhere else.

However again I'm not sure the result is what we want: The problem is (still/again) that when you have many labels in one category, the white container expands to accommodate them, potentially pushing the checkboxes outside the visible area and making you scroll, instead of having to scroll within the white container itself to reach all the categories.

@dagraf can you clarify what the requirements are?

@dagraf
Copy link
Collaborator

dagraf commented Dec 5, 2024

@JulianKniephoff The requirement as I see it would be: "A user can see the options to stop the video when this user does freshly load a video."
I can live with the solution as it is right now with but I am open to improvements. Do you have here an improvement in mind that could get implemented easily?

@JulianKniephoff
Copy link
Member

I don't really have anything specific in mind. But yeah, the requirement as you stated it is not met by this change, when you have a sufficient amount of labels in a category, right? Am I doing something wrong? 🤔

@dagraf
Copy link
Collaborator

dagraf commented Dec 11, 2024

Right. But as you do not have a specific improvement in mind which can meet the stated requirement and as I said that I can live with the current solution: We implement this solution and close this ticket.

@JulianKniephoff
Copy link
Member

Fair. :)

I would still say that we should just get rid of .categories-container-scroll in that case. Except @jduehring tells me it does actually change things.

@jduehring
Copy link
Contributor Author

@JulianKniephoff Agreed, I removed the container and as you said it doesn't seem to change anything else.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Options to stop video when writing/setting annotations are hidden
3 participants