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

feat: correct monarchs.json and add source information #596

Merged
merged 4 commits into from
Sep 6, 2024

Conversation

dsmedia
Copy link
Contributor

@dsmedia dsmedia commented Aug 19, 2024

Resolves #595

Before approving, please review how the dataset corrections will alter the example visualization Wheat and Wages appearing in the Vega, Vega-Lite and Altair example galleries.

Original from vega-lite gallery
Modified by hard-coding proposed changes

@domoritz
Copy link
Member

D3 example: https://mbostock.github.io/protovis/ex/wheat-full.html
Protovis: https://mbostock.github.io/protovis/ex/wheat.html

Original image from Playfair:
image

Copy link
Member

@domoritz domoritz left a comment

Choose a reason for hiding this comment

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

  • Having Elizabeth start earlier makes the Vega-Lite chart extend the domain. We should fix the domain before accepting this change. Similar with the end.

I suspect the data was made specifically for this chart so the start and end doesn't really matter. If we want to change the data, we should update the examples in Vega, Vega-Lite, and Altair before merging.

@dsmedia
Copy link
Contributor Author

dsmedia commented Aug 22, 2024

It's simple enough to avoid any breaking changes in any dependent visualizations by revising the SOURCES.md file to acknowledge that "the dataset contains two intentional inaccuracies to maintain compatibility with" Wheat and Wages. I didn't include links to the D3 or protovis examples but please feel free to do so if you'd like.

data/monarchs.json Outdated Show resolved Hide resolved
{"name":"James I","start":1603,"end":1625,"index":1},
{"name":"Charles I","start":1625,"end":1649,"index":2},
{"name":"Cromwell","start":1649,"end":1660,"commonwealth":true,"index":3},
{"name":"Charles II","start":1660,"end":1685,"index":4},
{"name":"James II","start":1685,"end":1689,"index":5},
{"name":"James II","start":1685,"end":1688,"index":5},
{"name":"W&M","start":1689,"end":1702,"index":6},
Copy link
Member

Choose a reason for hiding this comment

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

https://www.hrp.org.uk/kensington-palace/history-and-stories/william-iii-and-mary-ii/#gs.e3lczb says

William III and Mary II were England’s first and only joint sovereigns, with Mary sharing equal status and power. William and Mary came to the throne after the "Glorious Revolution" of 1688 when Mary’s father, James II, was deposed for trying to enforce Catholic tolerance in England. The King and Queen ruled jointly from 1689 until Mary’s death aged 32 in 1694.

Is 1702 correct?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

While William & Mary's joint rule ended in 1694, William's solo reign continued until his death in 1702, when he was thrown from a horse and broke his collar bone. This is noted in the proposed SOURCES.md entry as follows:

William & Mary's Reign

The entry "W&M" represents the joint reign of William III and Mary II. While the dataset shows their reign as 1689-1702, the official Web site of the British royal family indicates that Mary II's reign ended in 1694, though William III continued to rule until 1702.

The dataset was created prior to the reign of Elizabeth II
Copy link
Member

@domoritz domoritz left a comment

Choose a reason for hiding this comment

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

Thanks for the edits. Just one change.

SOURCES.md Outdated Show resolved Hide resolved
Co-authored-by: Dominik Moritz <domoritz@gmail.com>
@domoritz domoritz merged commit 2f62800 into vega:main Sep 6, 2024
2 checks passed
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.

Correct errors in monarchs.json or document them in SOURCES.md?
2 participants