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

improve drivers #856

Merged
merged 42 commits into from
Jan 2, 2024
Merged

improve drivers #856

merged 42 commits into from
Jan 2, 2024

Conversation

ShellyDCMS
Copy link
Collaborator

No description provided.

@ShellyDCMS
Copy link
Collaborator Author

@HarelM , can you please push a lockfile? Even when creating a lockfile with codespaces the build fails

@HarelM
Copy link
Collaborator

HarelM commented Dec 30, 2023

My codespaces account is maxed out for this month... :-/
What steps are you doing to generate the lock file in codespaces?
I think the following should work:

  1. nvm install to get the right node version
  2. npm install
  3. Push the lock file.

Did you try that?
You can also run npm ci to check that the lock file is correct.

@ShellyDCMS
Copy link
Collaborator Author

@HarelM ,
I delete the lock file,
I delete node modules
I run npm i
Can you try locally maybe?

Copy link

codesandbox-ci bot commented Dec 31, 2023

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit 6650262:

Sandbox Source
maputnik Configuration

@HarelM
Copy link
Collaborator

HarelM commented Dec 31, 2023

I did the above with the addition of npm cache clean --force to avoid any local cache issues. I also removed the cache part of the build which I don't think is necessary...

@codecov-commenter
Copy link

codecov-commenter commented Dec 31, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (124ae98) 57.18% compared to head (6650262) 57.15%.

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #856      +/-   ##
==========================================
- Coverage   57.18%   57.15%   -0.04%     
==========================================
  Files         104      104              
  Lines        2929     2929              
  Branches      674      674              
==========================================
- Hits         1675     1674       -1     
- Misses       1254     1255       +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

cypress/e2e/modals.cy.ts Outdated Show resolved Hide resolved
cypress/e2e/modals.cy.ts Outdated Show resolved Hide resolved
@HarelM
Copy link
Collaborator

HarelM commented Dec 31, 2023

Layes tests were a bit hard to review, I hope it's similar as I was not able to fully compare one test to the other.
Generally speaking this looks great!
THANKS!

@HarelM
Copy link
Collaborator

HarelM commented Jan 1, 2024

I really need to find a way to fix this lint rule for the switch case... :-/
I also like 4 spaces and not 2.
out of scope for this PR...

@ShellyDCMS ShellyDCMS marked this pull request as ready for review January 1, 2024 14:36
cypress/e2e/layers.cy.ts Outdated Show resolved Hide resolved
cypress/e2e/layers.cy.ts Outdated Show resolved Hide resolved
Copy link
Collaborator

@HarelM HarelM left a comment

Choose a reason for hiding this comment

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

Generally approved, THANKS!
I have reviewed and made sure the tests in layers.cy.ts are the same.
I have left a few minor comments.

@HarelM HarelM merged commit 8e35ed9 into maplibre:main Jan 2, 2024
8 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.

3 participants