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

Refactor useViewportFeatures #238

Merged
merged 13 commits into from
Dec 13, 2021
Merged

Conversation

Clebal
Copy link
Contributor

@Clebal Clebal commented Dec 9, 2021

No description provided.

@Clebal Clebal requested a review from VictorVelarde December 9, 2021 19:23
@coveralls
Copy link
Collaborator

coveralls commented Dec 9, 2021

Pull Request Test Coverage Report for Build 1573320190

  • 34 of 71 (47.89%) changed or added relevant lines in 5 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.4%) to 69.785%

Changes Missing Coverage Covered Lines Changed/Added Lines %
packages/react-api/src/hooks/utils.js 0 4 0.0%
packages/react-api/src/hooks/useFeaturesCommons.js 11 18 61.11%
packages/react-api/src/hooks/useGeoJsonFeatures.js 8 17 47.06%
packages/react-api/src/hooks/useTilesetFeatures.js 11 28 39.29%
Totals Coverage Status
Change from base Build 1559951290: 0.4%
Covered Lines: 931
Relevant Lines: 1248

💛 - Coveralls

@Clebal Clebal requested a review from padawannn December 10, 2021 08:26
setSourceViewportFeaturesReady(false);
executeTask(sourceId, Methods.LOAD_GEOJSON_FEATURES, { geojson })
.then(() => setGeoJsonLoaded(true))
.catch(throwError);
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here. I don't mind a lot about then.catch, but what about the previous catch errors we had?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Same answer.

Copy link
Contributor

Choose a reason for hiding this comment

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

I didn't see the custom throw. Agree that having it with then is more compact, thus probably more readable in this case.

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree with @VictorVelarde, about using await instead of then because it goes more in line with our style guides but it is a suggestion

Copy link
Contributor

@VictorVelarde VictorVelarde left a comment

Choose a reason for hiding this comment

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

Just small comments, but LGTM. I think it's a nice idea to have them separated.
My only concern is the error management we had and now it's gone.

Let's have tomorrow another quick CR and talk and then we can merge.

@Clebal
Copy link
Contributor Author

Clebal commented Dec 12, 2021

@VictorVelarde error management isn't gone, still there. It was just generalised for having the same one in both hooks.

@VictorVelarde
Copy link
Contributor

We will check with @padawannn tomorrow before the merge

Copy link
Contributor

@padawannn padawannn left a comment

Choose a reason for hiding this comment

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

LGFM, some suggestions

packages/react-api/src/hooks/useGeoJsonFeatures.js Outdated Show resolved Hide resolved
setSourceViewportFeaturesReady(false);
executeTask(sourceId, Methods.LOAD_GEOJSON_FEATURES, { geojson })
.then(() => setGeoJsonLoaded(true))
.catch(throwError);
Copy link
Contributor

Choose a reason for hiding this comment

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

I agree with @VictorVelarde, about using await instead of then because it goes more in line with our style guides but it is a suggestion

packages/react-api/src/hooks/useGeoJsonFeatures.js Outdated Show resolved Hide resolved
Copy link
Contributor

@VictorVelarde VictorVelarde left a comment

Choose a reason for hiding this comment

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

🚀 nice job!

@Clebal Clebal merged commit ed5ce91 into master Dec 13, 2021
@Clebal Clebal deleted the sclebal/refactor-useViewportFeatures branch December 13, 2021 14:54
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