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

O3-953: Location picker does not show all locations #343

Merged
merged 8 commits into from
Mar 16, 2022
Merged

O3-953: Location picker does not show all locations #343

merged 8 commits into from
Mar 16, 2022

Conversation

vasharma05
Copy link
Member

@vasharma05 vasharma05 commented Mar 3, 2022

Requirements

  • This PR has a title that briefly describes the work done, including the ticket number if there is a ticket.

For changes to apps

If applicable

  • My work includes tests or is validated by existing tests.
  • I have updated the esm-framework mock to reflect any API changes I have made.

Summary

This PR is still a work in progress.
I have added infinite scroll to fetch the multiple sets of locations in the location picker. Along with that, I have added backend connectivity to the search bar.

Screenshots

https://www.awesomescreenshot.com/video/7704971?key=67e59ee20d21e2a45aac94fd502e19d9

Related Issue

https://issues.openmrs.org/browse/O3-953

Other

@vasharma05 vasharma05 marked this pull request as draft March 3, 2022 18:06
@ibacher
Copy link
Member

ibacher commented Mar 3, 2022

@vasharma05 Very cool demo!

@github-actions
Copy link
Contributor

github-actions bot commented Mar 3, 2022

File size impact

Merging O3-953 into master impact files as follow:

@openmrs/esm-devtools-app (no impact)
Files new size
Unmodified (5) 436 kB (0 B / +0%) 👻
Total (5) 436 kB (0 B / +0%) 👻
@openmrs/esm-implementer-tools-app (no impact)
Files new size
Unmodified (21) 2.35 MB (0 B / +0%) 👻
Total (21) 2.35 MB (0 B / +0%) 👻
@openmrs/esm-login-app (+0.61%)
Files new size
packages/apps/esm-login-app/dist/375.js 94.9 kB 👶
packages/apps/esm-login-app/dist/762.js 20.7 kB 👶
packages/apps/esm-login-app/dist/747.js 88.6 kB (+2.33 kB / +2.71%) ↗️
packages/apps/esm-login-app/dist/770.js 3.38 kB (+144 B / +4.44%) ↗️
packages/apps/esm-login-app/dist/openmrs-esm-login-app.js 11.5 kB (+144 B / +1.27%) ↗️
packages/apps/esm-login-app/dist/574.js 867 B (+20 B / +2.36%) ↗️
packages/apps/esm-login-app/dist/593.js deleted (-10.7 kB)
packages/apps/esm-login-app/dist/309.js deleted (-95.4 kB)
Unmodified (22) 1.81 MB (0 B / +0%) 👻
Total (30) 2.03 MB (+12.2 kB / +0.61%) ↗️
@openmrs/esm-offline-tools-app (no impact)
Files new size
Unmodified (26) 2.78 MB (0 B / +0%) 👻
Total (26) 2.78 MB (0 B / +0%) 👻
@openmrs/esm-primary-navigation-app (no impact)
Files new size
Unmodified (18) 2.15 MB (0 B / +0%) 👻
Total (18) 2.15 MB (0 B / +0%) 👻
@openmrs/esm-app-shell (+0%)
Files new size
packages/shell/esm-app-shell/dist/service-worker.js 164 kB (+89 B / +0.05%) ↗️
Unmodified (3) 1.95 MB (0 B / +0%) 👻
Total (4) 2.12 MB (+89 B / +0%) ↗️
Generated by @jsenv/file-size-impact during Report bundle size#1988095351 on 8c9a723

Copy link
Member

@ibacher ibacher left a comment

Choose a reason for hiding this comment

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

A few small comments, but good work so far!

@vasharma05
Copy link
Member Author

Hi @brandones, @ZacButko!
I have removed the location-picker's test file since there are multiple changes done in the respective file. I need some help in writing unit tests since I don't have knowledge of the same.

@vasharma05
Copy link
Member Author

I had a conversation with @denniskigen about helping me out with the writing tests.
We'll be having a look at the tests tomorrow.

@vasharma05 vasharma05 marked this pull request as ready for review March 4, 2022 18:00
@vasharma05
Copy link
Member Author

Hi @ZacButko!
Can you please review the above 3 commits, I have replaced my code with SWR Infinite.
Thanks!

Copy link
Contributor

@ZacButko ZacButko left a comment

Choose a reason for hiding this comment

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

Looks good. Please fix missing / excessive hook dependencies before merging

Copy link
Member

@ibacher ibacher left a comment

Choose a reason for hiding this comment

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

Few more things. Nice work, though, Vineet!

@vasharma05
Copy link
Member Author

@ZacButko! Please have a review and if it looks good, please approve the PR.
Thanks!

@ibacher ibacher dismissed ZacButko’s stale review March 16, 2022 14:21

Dismissing per Zac's verbal approval

@ibacher
Copy link
Member

ibacher commented Mar 16, 2022

Tests to be added in subsequent PR

@ibacher ibacher merged commit 6b93e02 into openmrs:master Mar 16, 2022
@vasharma05
Copy link
Member Author

Thank you @denniskigen for writing the tests at #361.

@vasharma05 vasharma05 deleted the O3-953 branch March 17, 2022 16:39
@ZacButko ZacButko mentioned this pull request May 3, 2022
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