Skip to content

Add toggleable provinces legend#1471

Open
Avengium wants to merge 1 commit into
Azgaar:masterfrom
Avengium:province-legend
Open

Add toggleable provinces legend#1471
Avengium wants to merge 1 commit into
Azgaar:masterfrom
Avengium:province-legend

Conversation

@Avengium

Copy link
Copy Markdown
Contributor

This PR adds a new Provinces Legend toggle. A new button (#provincesLegend) in the provinces toolbar between [chart] and [provincelabels]. This button calls function toggleProvincesLegend handler.

This function has some things set up.

  • clears the legend if present or builds it from pack.provinces (only entries with p.i and not removed),
  • applies the current state filter (provincesFilterState),
  • maps provinces to [id, color, name],
  • and calls drawLegend("Provinces", data) to render it.

Add a new Provinces Legend toggle: insert a button (#provincesLegend) in the provinces toolbar and wire it to a new toggleProvincesLegend handler.

The handler clears the legend if present or builds it from pack.provinces (only entries with p.i and not removed), applies the current state filter (provincesFilterState), maps provinces to [id, color, name], and calls drawLegend("Provinces", data) to render it.
@netlify

netlify Bot commented Jun 11, 2026

Copy link
Copy Markdown

Deploy Preview for afmg ready!

Name Link
🔨 Latest commit a74f5c1
🔍 Latest deploy log https://app.netlify.com/projects/afmg/deploys/6a2af97b6919c600081c2aab
😎 Deploy Preview https://deploy-preview-1471--afmg.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify project configuration.

@Azgaar Azgaar self-requested a review June 12, 2026 12:47
@Azgaar

Azgaar commented Jun 12, 2026

Copy link
Copy Markdown
Owner

If I have all provinces displayed (states: all), then the legend doesn't fit the screen. We would need it to have a logic to change number of columns based on elements number and not go off screen.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

@Avengium

Copy link
Copy Markdown
Contributor Author
  1. What logic do you suggest for when a user has selected "States: All" in the Provinces editor?
  2. What specific considerations could you take for those cases where users have many states? (up to a hundred or more)
  3. Should we allow a legend for hundreds of provinces?

@Azgaar

Azgaar commented Jun 12, 2026

Copy link
Copy Markdown
Owner

We can allow it, but maybe rescale (change sizes inside). Or test how many still look good and restrict it to take x first max.

@Avengium

Copy link
Copy Markdown
Contributor Author

public\modules\ui\editors.js is a file that all legend toggle uses. If I modify something in editors.js, it would affect the behavior of the other legends as well. But for those, you already have standards and assumptions. So I'm not sure about modifying editors.js.

I had the idea of ​​adding a filter, "Provinces displayed on screen" [in the viewport], to filter the number of provinces instead of modifying the number of legend items that can be displayed simultaneously.

I think that setting an arbitrary limit based on the number of legend items that fit on the screen could cause different effects simply by changing the screen ratio and resolution. And I don't know what to do with that information.

Another filter we can add to the Provinces Editor is "Dropdown -> culture" and filter by cultures. This is similar to how it already works in the Burgs editor.

I think that even so, none of this completely solves the legend issue, and arbitrary adjustments will have to be made. These adjustments should be at the discretion of the repository owner.

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.

3 participants