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

Allow setting ViewerOptions and set default zoom limits #296

Merged
merged 1 commit into from Nov 8, 2023

Conversation

planger
Copy link
Member

@planger planger commented Nov 3, 2023

Sprotty introduced zoom limits, so we should allow setting them.
This change allows to optionally specify all ViewerOptions in configureDiagramOptions and sets very generous zoomLimits by default. This should also mitigate the bug when you zoom beyond Number.maxValue.

@tortmayr
Copy link
Contributor

tortmayr commented Nov 5, 2023

I'm not a fan of exposing partial IViewer options via the IDiagramOptions interface. With this approach
there is a potential mismatch if an adopter overrides the zoomLimit value in the viewer options. (IDIagramOptions than still holds the original value)

In my opinion the configureDiagramOptions method should take options of type IDiagramOptions & Partial<ViewerOptions>.
Alternatively we could also make the DiagramOptions interface a subinterface of ViewerOptions and rebind the ViewerOptions service identfier.

Sprotty introduced zoom limits, so we should allow setting them.
This change allows to optionally specify all `ViewerOptions` in
`configureDiagramOptions` and sets very generous `zoomLimits` by
default. This should also mitigate the bug when you zoom beyond
`Number.maxValue`.
@planger planger changed the title Allow setting zoom limits Allow setting ViewerOptions and set default zoom limits Nov 7, 2023
@planger planger requested a review from tortmayr November 7, 2023 19:39
@planger
Copy link
Member Author

planger commented Nov 7, 2023

Good comment, I agree that this approach wasn't very versatile. However combining IDiagramOptions and Partial<ViewerOptions> isn't very nice either because we'd have to explicitly pass the set ViewerOptions to avoid inadvertantly adding IDiagramOptions properties into the options.

Thus, I opted to introduce an optional second argument. WDYT?

Copy link
Contributor

@tortmayr tortmayr left a comment

Choose a reason for hiding this comment

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

Thanks. Looks good to me. I like the approach with a second options argument for the viewer options. 👍🏼

@planger planger merged commit f5bcc1c into master Nov 8, 2023
6 checks passed
@planger planger deleted the allow-setting-zoom-limits branch November 8, 2023 07:58
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.

None yet

2 participants