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

Use a map for Onyx collection keys #330

Merged
merged 1 commit into from
Sep 11, 2023
Merged

Use a map for Onyx collection keys #330

merged 1 commit into from
Sep 11, 2023

Conversation

ospfranco
Copy link
Contributor

Details

This change was proposed, merged and reverted due to a failing test and some misscommunication. I've fixed the initial value of the collection map and that also fixes the tests.

Revert PR

This change also provides an improvements to the ReportScreen optimzations Margelo is currently working on.

Related Issues

#329

Automated Tests

Linked PRs

@ospfranco ospfranco requested a review from a team as a code owner September 6, 2023 09:14
@melvin-bot melvin-bot bot requested review from cead22 and removed request for a team September 6, 2023 09:15
Copy link
Collaborator

@tgolen tgolen left a comment

Choose a reason for hiding this comment

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

Ah, using Map(). Nice!

@tgolen
Copy link
Collaborator

tgolen commented Sep 6, 2023

Could you please add screenshots showing that the App tests are passing with this change and the app builds?

Copy link

@cead22 cead22 left a comment

Choose a reason for hiding this comment

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

Could you please add screenshots showing that the App tests are passing with this change and the app builds?

Approving, but waiting for Tim's request to be addressed before we merge

@ospfranco
Copy link
Contributor Author

Here are the passing tests

CleanShot 2023-09-06 at 10 35 38

Here is a screenshot of metro and the app running

CleanShot 2023-09-07 at 09 01 53

@cead22
Copy link

cead22 commented Sep 7, 2023

@tgolen I think your concern is addressed. Wanna hit the button or should we HOLD this one for the feature freeze? Probably the latter

@mountiny
Copy link
Contributor

Going ahead and ship this since we got all the approvals, thanks!

@mountiny mountiny merged commit 3c2fa91 into Expensify:main Sep 11, 2023
3 checks passed
@ospfranco ospfranco deleted the osp/collection-keys-map branch September 12, 2023 09:50
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