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 maxZoom #2557

Merged
merged 6 commits into from
May 7, 2024
Merged

Fix maxZoom #2557

merged 6 commits into from
May 7, 2024

Conversation

StyXman
Copy link
Contributor

@StyXman StyXman commented May 6, 2024

The documentation says:

and zoomed to the required level of detail using an integer from 0 (for global detail) to 20 (for building level detail)

but the HTML declares a maxZoom of 19.

PR Checklist:

  • All new features have been tested
  • All new features have been documented
  • I have read the CONTRIBUTING.md file
  • I will abide by the code of conduct

The documentation says:

> and zoomed to the required level of detail using an integer from 0 (for global detail) to 20 (for building level detail)

but the HTML declares a `maxZoom` of 19.
@StyXman
Copy link
Contributor Author

StyXman commented May 6, 2024

Alternatively we could change the docs.

@StyXman
Copy link
Contributor Author

StyXman commented May 6, 2024

Is it 'normal' that towncrier fails?

@freakboy3742
Copy link
Member

Is it 'normal' that towncrier fails?

Yes - if you haven't provided a change note

Copy link
Member

@freakboy3742 freakboy3742 left a comment

Choose a reason for hiding this comment

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

Thanks; In addition to the change note requirement that I flagged in a separate comment, the same change will also need to be made to the Winforms backend, as it also uses the embedded web view approach.

The documentation mentions 20, but here it's hardcoded to 19.
Copy link
Member

Choose a reason for hiding this comment

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

This filename should be 2557.misc.rst to placate the town crier check.

@StyXman
Copy link
Contributor Author

StyXman commented May 7, 2024

I don't quite understand the thing about the towncrier file. The docs say:

every pull request needs to have a corresponding file in the changes/ directory

It doesn't mention how it should be named or what format.

You can also see existing examples of news fragments in the changes/ folder

The folder is empty except for the template that collects them all in one file, I guess.

@freakboy3742
Copy link
Member

I don't quite understand the thing about the towncrier file. The docs say:

every pull request needs to have a corresponding file in the changes/ directory

It doesn't mention how it should be named or what format.

Those instructions could probably be clearer; it links to the Towncrier docs, which IIRC used to be a lot more concise about what filenames and content to use. As noted in my comment inline - it should be <ticket_number>.<type>.rst - so 2557.misc.rst in this case (since it's a minor change). It could be 2557.bugfix.rst if you feel it's worth a specific call out in the release notes - my inclination is that it isn't, but I'm willing to be convinced otherwise.

@StyXman
Copy link
Contributor Author

StyXman commented May 7, 2024

Those instructions could probably be clearer

I'll try some rephrasing.

@StyXman
Copy link
Contributor Author

StyXman commented May 7, 2024

Is it normal that

The folder is empty except for the template that collects them all in one file, I guess.

?

@StyXman
Copy link
Contributor Author

StyXman commented May 7, 2024

Those instructions could probably be clearer

I'll try some rephrasing.

WIP, will propose another PR:

StyXman/toga@mapview-maxZoom-fix...contribute-code-towncrier

I still need some clarification about the emptiness of the changes/ directory.

@freakboy3742
Copy link
Member

Only when you submit a pull request right after we cut a release :-)

If this PR had been submitted 24 hours ago, that folder would have had a hundred files in it. So, that's an edge case, to be sure.

@StyXman StyXman requested a review from freakboy3742 May 7, 2024 10:50
@freakboy3742
Copy link
Member

Thanks for the fix!

@freakboy3742 freakboy3742 merged commit 3264094 into beeware:main May 7, 2024
34 checks passed
@StyXman StyXman deleted the mapview-maxZoom-fix branch May 9, 2024 09:36
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.

2 participants