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

Fit* values belong to view in URL parameters #15741

Closed
wants to merge 1 commit into from

Conversation

ousia
Copy link

@ousia ousia commented Nov 24, 2022

@calixteman
Copy link
Contributor

It'd be better to first file a bug and then submit a patch.
I didn't find why Fit** are got from the zoom parameter:
#5947
I just tested an url in Chrome with the builtin viewer and it works correctly when Fit** is in view but not when in zoom.
@Snuffleupagus is it a bug or do you remember if it has been made on purpose ?

@Snuffleupagus
Copy link
Collaborator

It'd be better to first file a bug and then submit a patch.

Well, there's the old issue #10773 however if we change this now it might be breaking someones use-case. The original, but not fully working, implementation landed in PR #3910 as far as I can tell.

So while we should perhaps change this, there's always the risk of regressions; furthermore #10773 (comment) mentioned not knowing JavaScript and as a reviewer that scares me (since it's not even clear that this has been successfully tested locally).

Personally, I'd very much like to defer a decision/reviewing to @timvandermeij since I don't really have time to dig into the details here.

@timvandermeij
Copy link
Contributor

I agree with what's written above, and at the moment I also don't have time to dive into the details. Let's close this but keep #10773 open since I do feel that the specification is not implemented correctly right now, but I'd be much more comfortable with a PR that at least has some tests covering this behavior since this is quite a big change to land without any test coverage unfortunately.

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.

4 participants