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 Timeline Slider Visibility in Status Bar for Certain Documents #423

Merged
merged 7 commits into from
Jan 9, 2025

Conversation

Meriem-BenIsmail
Copy link
Contributor

This PR resolves an issue where the timeline slider in the status bar was not appearing for certain documents. The problem stemmed from overly restrictive checks in the isActive method of the statusBarTimeline plugin.

Copy link
Contributor

github-actions bot commented Jan 6, 2025

Binder 👈 Launch a Binder on branch Meriem-BenIsmail/jupyter-collaboration/bug-timeline

Copy link
Collaborator

@davidbrochart davidbrochart left a comment

Choose a reason for hiding this comment

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

So it seems that this PR just doesn't check that the shared model has a document_id anymore. Could you explain why this check was here in the first place, and in which case a shared model may not have a document_id?

@Meriem-BenIsmail
Copy link
Contributor Author

So it seems that this PR just doesn't check that the shared model has a document_id anymore. Could you explain why this check was here in the first place, and in which case a shared model may not have a document_id?

The document_id check was originally added to ensure the timeline slider interacted with uniquely identifiable collaborative documents.
However, this check caused issues for non-notebook documents because their shared models typically don’t define a document_id (in the example of text files, markdown files..., we can't find the document_id)

By removing the check, the timeline slider now works more broadly with all collaborative documents, not just those with a document_id.

@davidbrochart
Copy link
Collaborator

Thanks @Meriem-BenIsmail. I was also curious about this check: typeof currentWidget.context.path === 'string'.

@Meriem-BenIsmail
Copy link
Contributor Author

I was also curious about this check: typeof currentWidget.context.path === 'string'

Oh that was first added when I was going to check for the RTC prefix in the path later. I'll remove it

@krassowski krassowski added the bug Something isn't working label Jan 9, 2025
@krassowski krassowski merged commit 081e588 into jupyterlab:main Jan 9, 2025
27 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants