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

Attribution default open for osm #795

Merged
merged 50 commits into from
Feb 10, 2022

Conversation

acalcutt
Copy link
Contributor

@acalcutt acalcutt commented Jan 19, 2022

This is an attempt to Show OSM attribution by default, like mentioned in ( #205 )

This change makes the compact mode default to being open to comply with the OpenSteetMap Attribution Guidelines that expect attribution is visible for at least some time without user interaction to fall within the safe harbour rules.

  • code has been added so the button is default open when going into compact mode.

  • code has been added so attribution will close automatically when there is a drag action

  • code has been added to not show the compact button if there are no attributes to prevent a open attribute button with no attributes. (I actually noticed the compact button existing when there were no attributes bug on the maplibre site on <= 640 screen width, but it is more noticeable when it is open by default. )

  • Confirm your changes do not include backports from Mapbox projects (unless with compliant license) - if you are not sure about this, please ask!

  • Briefly describe the changes in this PR.

  • Include before/after visuals or gifs if this PR includes visual changes.

  • Write tests for all new functionality.

  • Document any changes to public APIs.

  • Post benchmark scores.

  • Manually test the debug page.

  • Suggest a changelog category: bug/feature/docs/etc. or "skip changelog".

  • Add an entry inside this element for inclusion in the maplibre-gl-js changelog: <changelog></changelog>.

acalcutt and others added 24 commits December 6, 2021 11:34
Default compact button to be open by default, to make OpenStreetMap attibutions to show by default like mentioned in ( maplibre#205 )

This uses an idea from ( maplibre#205 (comment) ) to show compact open by default, but close it when dragged.
@github-actions
Copy link
Contributor

github-actions bot commented Jan 19, 2022

Bundle size report:

Size Change: +80 B
Total Size Before: 194 kB
Total Size After: 194 kB

Output file Before After Change
maplibre-gl.js 185 kB 185 kB +80 B
maplibre-gl.css 9.43 kB 9.43 kB 0 B
ℹ️ View Details
Source file Before After Change
rollup/build/tsc/src/ui/control/attribution_control.js 1.01 kB 1.08 kB +74 B

Most of these fails because I made the compact button not show if attributes are empty.

The other test fail because when switching between >640 and <=640 the open attribute is now added
@acalcutt acalcutt marked this pull request as ready for review January 20, 2022 04:16
@acalcutt
Copy link
Contributor Author

@HarelM
Copy link
Collaborator

HarelM commented Feb 5, 2022

Please update changelog file.

@acalcutt
Copy link
Contributor Author

acalcutt commented Feb 6, 2022

I updated the changelog and did a re-merge, but it seems like some other stuff I didn't touch in "test/integration/assets/sprites/ " and "build/generate-query-test-fixtures.ts" are causing issues now.

not sure what I should do to fix those things since they arn't part of what I changed

@wipfli
Copy link
Contributor

wipfli commented Feb 8, 2022

Can you share the diffs of the failing render tests?

@wipfli
Copy link
Contributor

wipfli commented Feb 8, 2022

Can you run the render tests locally, go to the folder of the failing tests, and check for the file diff.png? There should be these files after the tests run: expected.png style.json diff.png actual.png

@acalcutt
Copy link
Contributor Author

acalcutt commented Feb 8, 2022

Thanks everyone, I was able to fix the test-render by restoring those sprites with

git checkout main -- test/integration/assets/sprites/[email protected]
git checkout main -- test/integration/assets/sprites/[email protected]
git checkout main -- test/integration/assets/sprites/icon-text-fit-2x.json
git checkout main -- test/integration/assets/sprites/icon-text-fit-2x.png

This is finally passing again.

@acalcutt
Copy link
Contributor Author

acalcutt commented Feb 8, 2022

I upgraded the example page at ( https://stackblitz.com/edit/web-platform-ralmch?file=index.html ) to the latest version. still working as expected.

@wipfli
Copy link
Contributor

wipfli commented Feb 9, 2022

Thanks @acalcutt! @astridx would you mind having a look at this?

@astridx
Copy link
Contributor

astridx commented Feb 10, 2022

I think this PR is a great improvement. It's not just that the Attribution Control probably fulfils Openstreetmap licence better. It also corrects some things that I think were not intuitive before. Thank you for this @acalcutt
Here are my notes, if someone is interessed:

attributionControl in Map compact in AttributionControl customAttribution in AttributionControl ---
not set or true no Control no Control 1 Expectation: No attribution control is displayed. Neither above nor below 640 pixels. At the moment, an empty frame is displayed if the screen is smaller than 640. This PR corrects this.
not set or true false not set 2 Same as before
not set or true false small text 3 Expectation: Text is displayed in a non-compact form. Above and below 640 pixels screen width. This is now also super. Before, an empty compact frame was displayed below 640.
not set or true false big text 4 Same as before
not set or true true not set 5 Expectation: As there is no text, nothing should be shown. This is great in the version of these PRs. Before, the empty compact control was displayed. Under 640 pixels even in double form.
not set or true true small text 6 Expectation: The text is presented in a compact control because it is explicitly set. The Attribution Control was and is displayed in a compact form. In this PR, the display starts in open form and can be closed by the user. In my opinion, this is a good solution. It is admittedly a change of behaviour. But it is not a breaking change.
not set or true true big text 7 Same as before
not set or true - - 8 Same as 5
not set or true not set small text 9 Expectation: Text is displayed in a non-compact form above 640 Pixel because the option compact is not explicitly set. In our current version, the display switches to the closed compact version when the number of pixels is less than 640. This PR changes the switch to the open compact version ( and ensures that the empty control is no longer displayed. This is great!)
not set or true not set big text 10 Same as 8.
false no Control no Control 11 Expectation: No attribution control is displayed. That is fine bofore and actually.
false false - 12 Expectation: No attribution control is displayed as there is no text. That is fine bofore and actually below and above 640 Pixel.
false false small text 13 Expectation: Text is displayed in a non-compact form. That is fine bofore and actually below and above 640 Pixel.
false false big text 14 Same as 13
false true - 15 Expectation: As there is no text, nothing should be shown. This is fine in this PR. Before below 640 Pixel an empty compact control is shown.
false true small text 16 Expectation: The text is presented in a compact control because it is explicitly set. That was correct before and is correct now. The difference is that now the compact control starts in open form. See 6
false true big text 17 Same as 16.
false - - 18 Same as 15
false - small text 19 Expectation: Text is displayed in a non-compact form above 640 Pixel because the option compact is not explicitly set. In our current version, the display switches to the closed compact version when the number of pixels is less than 640. This PR changes the switch to the open compact version.
false - big text 20 Same as 18.

@HarelM HarelM merged commit b3213ee into maplibre:main Feb 10, 2022
wipfli added a commit that referenced this pull request Feb 24, 2022
* Migrate expression tests to jest (#965)

* Revert "Move benchmarks to ES modules (#964)" (#969)

This reverts commit aa8ed9d.

* Migrate query tests from puppeteer to playwright (#966)

* Fix web worker in watch mode (#968)

* Fix web worker in watch mode

* Add webworker support throughout

* Don't change rollup for style-spec and benchmarks

* Revert "Don't change rollup for style-spec and benchmarks"

This reverts commit 62a7e14.

* Remove duplicate node-resolve configuration

* Simplify build pipeline (#961)

* Remove build-tsc compile step

* re-enable failing style-spec test

* cleanup

* cleanup

* fix style-spec-test

* Create codeql-analysis.yml

* Bump simple-get from 3.1.0 to 3.1.1 (#971)

Bumps [simple-get](https://github.com/feross/simple-get) from 3.1.0 to 3.1.1.
- [Release notes](https://github.com/feross/simple-get/releases)
- [Commits](feross/simple-get@v3.1.0...v3.1.1)

---
updated-dependencies:
- dependency-name: simple-get
  dependency-type: indirect
...

Signed-off-by: dependabot[bot] <[email protected]>

Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

* Bump nanoid from 3.1.30 to 3.2.0 (#973)

Bumps [nanoid](https://github.com/ai/nanoid) from 3.1.30 to 3.2.0.
- [Release notes](https://github.com/ai/nanoid/releases)
- [Changelog](https://github.com/ai/nanoid/blob/main/CHANGELOG.md)
- [Commits](ai/nanoid@3.1.30...3.2.0)

---
updated-dependencies:
- dependency-name: nanoid
  dependency-type: indirect
...

Signed-off-by: dependabot[bot] <[email protected]>

Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

* Bump cached-path-relative from 1.0.2 to 1.1.0 (#972)

Bumps [cached-path-relative](https://github.com/ashaffer/cached-path-relative) from 1.0.2 to 1.1.0.
- [Release notes](https://github.com/ashaffer/cached-path-relative/releases)
- [Commits](https://github.com/ashaffer/cached-path-relative/commits)

---
updated-dependencies:
- dependency-name: cached-path-relative
  dependency-type: indirect
...

Signed-off-by: dependabot[bot] <[email protected]>

Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

* Move browser test to playwright (#974)

* Bump ts-node for 3x faster postinstall (#975)

* Remove web worker replacement (#976)

* Attribution default open for osm (#795)

* Update CHANGELOG.md

* Revert "Update CHANGELOG.md"

This reverts commit 0b81a41.

* attribution fixes (from astridx)

a1272d9

b9b0370

* Update .gitignore

* fix missing new line lint complained about

* Revert "attribution fixes (from astridx)"

This reverts commit 2031d8e.

* Default compact to be open by default

Default compact button to be open by default, to make OpenStreetMap attibutions to show by default like mentioned in ( #205 )

This uses an idea from ( #205 (comment) ) to show compact open by default, but close it when dragged.

* remove test string I forgot to remove

* fix compact button showing when there are no attributions

* update whitespace trim to es6 syntax

* fixes after testing various states of compact

* revert back so it is open when starting in fullscreen

* revert .gitignore

* fix lint errors

* fix tests

Most of these fails because I made the compact button not show if attributes are empty.

The other test fail because when switching between >640 and <=640 the open attribute is now added

* lint

* Update package-lock.json

* Update package-lock.json

* Update CHANGELOG.md

* delete problematic files

* put back deleted files

* test/integration/assets/sprites/[email protected]

* restore files

Co-authored-by: acalcutt <[email protected]>

* Fix benchmarks (#984)

* Move bench under `test` folder (#979)

* Add typeof guard to performance variable (#986)

* Fixes #768

* Fix comment and add changelog

* Release 2.1.2 Version (#987)

* Release 2.1.2 Version

* Fix changelog according to comments

* Fix release.yml due to bench folder move (#988)

* lint function-url-quotes/ (#983)

* Bump to v2.1.3 - Fix postinstall error `ts-node` not found in non-dev installs (#991)

* Bump to v2.1.4 - Fix missing `postinstall.js` file in npm publish (#992)

* Bump to v2.1.5-pre.1 - Publish empty `postinstall.js` file (#994)

* Bump to v2.1.5 (#996)

* Bump to v2.1.6-pre.1 - Fix `dist/package.json` (#998)

* Bump to v2.1.6 (#999)

* prefix (#1004)

* correct done (#1006)

* Add adjustment to rendering glyphs (#1005)

* add topAdjustment for glyph

* add topAdjustment for glyph

* update changelog

* add issue number

* add alphabet and cjk text test #1002

* fix lint error

* Simplify render tests (#1003)

Fixes #1008 

* Initial commit - half of the tests are failing

* Missing file

* Remove template rendering

* Fix usage of localizeUrls

* Move harness and server to render folder, simplify render code to be sepcific.

* Move function outside and add some typings

* lint

* Add typings, move compare results outside

* Remove server usage and replace it by file system reading.

* Move xhr mocking to main flow

* Move functions outside the render function

* Move creation of tests to render.ts, added more typings

* Move suite implementation into main flow file, add some typings.

* lint

* Moved all logic to a single file that runs the render tests

* remove color from harness

* Remove jest run command

* Small fixes

* lint

* Cleanup

* More cleanup

* Remove harness and move it to a single file. Remove the jest version.

* Remove ignore related code and tests

* Remove ignore, add types

* Add more types

* lint

* Migrate actual image generation to not use Buffer

* lint

* Fix CI run

* Fix CI properly this time...

* Revert png change

* Revert only buffer usage

* Revert png and buffer

* Simplify png in mock file

* Move to use uint8array and change throshold

* Added diff info

* remove specific threshold from test as it fails

* Fix typo

* Fix according to code review

* Husky pre-commit do not fix lint (#1019)

* Fix css

* Use main package-lock.json

Co-authored-by: Birk Skyum <[email protected]>
Co-authored-by: Yuri Astrakhan <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: Harel M <[email protected]>
Co-authored-by: Andrew Calcutt <[email protected]>
Co-authored-by: acalcutt <[email protected]>
Co-authored-by: vanilla-lake <[email protected]>
Co-authored-by: Astrid <[email protected]>
Co-authored-by: Kanahiro Iguchi <[email protected]>
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.

4 participants