Regression: Initial zoom level global config only accepts numbers, can't use strings "fit" or "fill"

Looking at https://photoswipe.com/adjusting-zoom-level/
I totally agree! This should be changed to accept any of the allowed values!

@nickbn could you prepare a MR? I think we should validate the acceptable values then.

PS: Default zoom level should be "fit"!

We should reset this regression in an update hook! Thanks for the report!

@anybody I'm building my first Drupal project in 10 years and I'm a little embarrassed to admit I don't know what an MR is. I'm happy to do anything in my skillset, but my skillset is small (so far). What's an MR?

@anybody also I see you moved this do dev and somehow I accidentally moved it back to rc5. Not my intention, my apologies.

@nickbn sorry. MR means merge-request. You can do it here as alternative to providing a patch. Do you think you'll be able to give it a try? Or aren't you a developer?

Otherwise we'll have to wait until someone has the time to fix this.

Found same thing as nickbn when I upgraded to rc4 > rc5. So I reverted back to rc4. The images were loading full size with rc5, regardless of the width of the browser window.

I prepared a MR to fix the issue. Could you please try if it solves the issue for you?

Okay seems to work fine in my tests, let's wait for further feedback.

Can confirm that is working here.

Thanks @Hephaestus, so let's set this RTBC!

Merged into dev!

I'll tag a new release.

This broke tests. Unfortunately, the new test indicator is quite small. Please adjust the tests accordingly in a follow-up issue and reclose this issue.

Oh wow, it didn't actually.... sorry!!

Ok, it did break tests. :P
Furthermore, we are changing the schema definition of "initialZoomLevel", "secondaryZoomLevel" and "maxZoomLevel", but only update the "initialZoomLevel" in our update hook? Does this even work? Will do we not need to cast the other options to string?

And lastly, I don't really like forcing "initialZoomLevel" to be "fit". The original setting should've simply be cast to string, as numbers are valid values as well, see https://photoswipe.com/adjusting-zoom-level/#supported-values.

But we tagged a new release already, so if the module still works all fine! :)

@Grevil: The settings are stored in yml anyway, I don't think it's as strict as in databases.

Re "fit": What should be the numeric representation of fit and fill?
The output was miserably broken using the number before, so setting this back to original PS value was urgent.

What should be the numeric representation of fit and fill?

There is none, but numeric values are also allowed, so if somebody wanted to have 2x Original Size for the "initialZoomLevel" (value: 2), this update will overwrite this setting with "fit" instead.

And if an allowed value would make the formatter "miserably broken", then we should create an issue in https://github.com/dimsemenov/PhotoSwipe. And disallow numeric strings.

Ok perhaps I got you wrong.
If someone enters an invalid value, it's his problem ;D String is fine in this case, as it's what the library accepts and it's JS, not typescript. No need to be over-compliant.

But the value is stored in config and configuration is heavily typed. And if we allow both strings like "fit" and numbers like 1 or 2, that should be reflected in the schema definition, where it currently isn't. But there is no way to add multiple schema types for one config entry.

And number are NOT invalid! If that is what you are referring to.

"2" == 2 in JavaScript in the end, so I still don't see the problem here. Beside being precise of course, but that's real world vs. perfection. I'm also a fan of perfection! :D

I think I just didn't get the point, we'll talk tomorrow :)

Yea, thought the settings form might throw some schema errors or something. But seems all fine! :)

Automatically closed - issue fixed for 2 weeks with no activity.