-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
[Chore] Dependency upgrade #1314
Conversation
9cb32f4
to
78333b1
Compare
// center being calculated by geo-vieweport.viewport has a complex logic that | ||
// projects and then unprojects the coordinates to determine the center | ||
// Calculating a simple average instead as that is the expected behavior in most of cases | ||
const center = [(bounds[0] + bounds[2]) / 2, (bounds[1] + bounds[3]) / 2]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Per the comment above, @mapbox/geo-viewport
has updated the way viewport is being determined, making the tests fail. It seems like this simplified implementation is what the expected behavior is, so I've revisited it accordingly. There doesn't seem to be a single place in which viewport is being calculated, which could be a potential improvement to encapsulate this type of logic. References:
viewport
function in v0.2.2 (pre-upgrade) - https://github.com/mapbox/geo-viewport/blob/v0.2.2/index.js#L24viewport
function in the lastermaster
(post-upgrade) - https://github.com/mapbox/geo-viewport/blob/master/index.js#L32
src/utils/interaction-utils.js
Outdated
@@ -156,11 +156,11 @@ export function getTooltipDisplayDeltaValue({ | |||
// safely cast string | |||
displayDeltaValue = defaultFormatter(displayDeltaValue); | |||
const deltaFirstChar = displayDeltaValue.charAt(0); | |||
if (deltaFirstChar !== '+' && deltaFirstChar !== '-') { | |||
if (deltaFirstChar !== '+' && deltaFirstChar !== '−') { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This one took some time to figure out, but tests were failing due to a change in d3-format
which now uses a minus sign (−
) instead of a hyphen (-
), which according to this issue is the correct way to provide better text alignment across different fonts.
I've made the updates to match this change and use the minus sign instead, if this is not what we want to do, according to the docs it should be possible to override the minus sign for a specific locale (relevant PR here)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Characters that look the same in code are an incredible trap door. At a minimum, can we use a Unicode escape sequence here ('\u2212'
) instead of the bare character, plus a comment, to save some time for the next developer to deal with this code?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wanted to get a take on which sign we actually want to use here - revisited the comparison as there were no objections to switching to the minus sign
@@ -62,7 +62,9 @@ test('Components -> injector -> injectComponents', t => { | |||
test('Components -> injector -> missing deps', t => { | |||
const spy = sinon.spy(Console, 'error'); | |||
|
|||
// eslint-disable-next-line react/display-name |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Warnings were coming up in tests due to missing displayName
@@ -2478,7 +2478,7 @@ test('#visStateReducer -> REMOVE_DATASET w filter and layer', t => { | |||
splitMaps: oldState.splitMaps, | |||
layerClasses: oldState.layerClasses, | |||
animationConfig: oldState.animationConfig, | |||
initialState: [], | |||
initialState: oldState.initialState, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tests were failing because []
was expected and the actual state was {}
, not sure how my changes affected this, but {}
seems like the right value?
@@ -68,6 +68,11 @@ Object.defineProperty(window, 'clipboardData', { | |||
writable: true | |||
}); | |||
|
|||
// These do not seem to be present under jsdom v16, even though the documentation suggests that should be the case | |||
['addEventListener', 'removeEventListener', 'dispatchEvent'].forEach(prop => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Spent a lot of time on this one, eventually had to patch it up like so as I am not sure what the underlying issue is exactly
Signed-off-by: Ilija Puaca <[email protected]>
78333b1
to
7cb5c48
Compare
@ilijapuaca i tried load an examples locally but they don't work: |
the netlify build works, you need to cleanup node modules in examples/demo folder |
I checked netlify build here: https://deploy-preview-1314--keplergl.netlify.app/demo/earthquakes | https://deploy-preview-1314--keplergl.netlify.app/demo/nyctrips and don't work. As far as local goes i did: |
Acutally you're right, looks like it's related to timeline range-brush component. either a change of API on |
Could be relevant |
@heshan0131 @macrigiuseppe patched up the issue which was introduced with d3/d3-selection#191 |
Signed-off-by: Ilija Puaca <[email protected]>
c452fd6
to
b7a2d37
Compare
Signed-off-by: Ilija Puaca <[email protected]>
Addressed an issue with |
looking |
I've made a pass on the dependency versions in order to bring them up to date as some of them got pretty outdated, which may present an issue for some of the adopters.
I've only upgraded the dependencies which seemingly did not have many breaking changes, in order to be able to batch them into a single PR. Upgrading libraries that may require more structured overhaul will be done in followup PRs.
There were a few issues that came up with the upgrades which I had to patch up, all of which I've provided in-line comments in this PR as to why specific changes had to be made.
Below is the list of libraries for which major version was bumped up, including a list of breaking changes based on their changelogs. The list is then followed by libraries which versions I did not upgrade, as they would make this PR harder to review.
Upgraded libraries
Libraries pending upgrade