tangrams / bubble-wrap

Bubble Wrap basemap style
http://tangrams.github.io/bubble-wrap/
MIT License
27 stars 21 forks source link

shields polish: USA and international #270

Closed nvkelso closed 5 years ago

nvkelso commented 6 years ago
tallytalwar commented 6 years ago

Request to put % usage in a separate commit or ideally in a separate PR, to make it easier for development comparison for ES.

nvkelso commented 5 years ago

Consider splitting USA and international shields into separate icon bundles that are like theme imports.

nvkelso commented 5 years ago

@sensescape and @bcamper this is ready for review, finally! :)

bcamper commented 5 years ago

The reason the 52f98b7f1c34f85547dcf72efb78b54095d0f717 commit didn't work is that it dropped the all while keeping the filters as arrays, which have an implicit any behavior. An object filter has implicit all behavior. E.g. see example in 6bb86d946d44c820b71167801a94575341dbdfea:

Should be filter: { network: [...], shield_text: true }

Or can be written as:

filter:
  network: [...]
  shield_text: true

But NOT as an array: filter: [ network: [...], shield_text: true ] or

filter:
  - network: [...]
  - shield_text: true

Does the difference make sense?

nvkelso commented 5 years ago

Hmm, it does now but wasn’t obvious before. I prefer to use the explicate all here as I find it less buggy / confusing.

On Tue, Oct 16, 2018 at 07:59 Brett Camper notifications@github.com wrote:

The reason the 52f98b7 https://github.com/tangrams/bubble-wrap/commit/52f98b7f1c34f85547dcf72efb78b54095d0f717 commit didn't work is that it dropped the all while keeping the filters as arrays, which have an implicit any behavior. An object filter has implicit all behavior. E.g. see example in 6bb86d9 https://github.com/tangrams/bubble-wrap/commit/6bb86d946d44c820b71167801a94575341dbdfea :

Should be filter: { network: [...], shield_text: true }

Or can be written as:

filter: network: [...] shield_text: true

But NOT as an array: filter: [ network: [...], shield_text: true ] or

filter:

  • network: [...]
  • shield_text: true

Does the difference make sense?

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/tangrams/bubble-wrap/pull/270#issuecomment-430271769, or mute the thread https://github.com/notifications/unsubscribe-auth/AA0EO_OF07ExqPUlcHkaL1IyJrlSIVblks5ulfRbgaJpZM4S_JYn .

bcamper commented 5 years ago

I definitely find the YAML difference between the "expanded" form (with - for arrays, but no preceding character for objects) confusing. For "simple all" filters I like the more compact form filter: { network: [...], shield_text: true }, but it is mostly a matter of preference. The extra all does have some parsing/memory overhead, but it's likely negligible.