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

feat: add accessControl field to Android Compose template #1385

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

xaviPastHubs
Copy link

@xaviPastHubs xaviPastHubs commented Nov 8, 2024

added accessControl to android compose template

It seems that the accessControl field is not controlled in the Compose/Object template (Android), unlike in ios-swift/any.swift for example. It could be added to change the visibility of the created objects, thereby improving the simplicity of development in apps that use it.

Issue link here

added accessControl to android compose template
Copy link

This pull request is automatically being deployed by Amplify Hosting (learn more).

Access this pull request here: https://pr-1385.d16eby4ekpss5y.amplifyapp.com

@xaviPastHubs xaviPastHubs changed the title Add the 'accessControl' field to the Android Compose template feat: add accessControl field to Android Compose template Nov 8, 2024
@jorenbroekema
Copy link
Collaborator

jorenbroekema commented Nov 8, 2024

Awesome, can you add a changeset (patch I guess since this is a bugfix imo) by running npx changeset locally and committing the results of that?

I would also expect a test to now fail, or if there is no test for this format yet, could you add a test? npm run test locally, or npm run test:watch to go into debugging mode. If the only failures are snapshot tests, then you can run npm run test:update-snapshots and check the diff with git diff or something and verify that the snapshot updates make sense and are what you expect as a consequence of your code change

@xaviPastHubs
Copy link
Author

@jorenbroekema I’ve followed the steps you provided and fixed the failing tests. I had to update the compose android snapshot as I also made a couple of formatting adjustments (semicolon after the package, a double space in the constructor, and a new line at the end of the file). Let me know if there’s anything else you need or if you have any further feedback!

jorenbroekema
jorenbroekema previously approved these changes Nov 12, 2024
@jorenbroekema
Copy link
Collaborator

I added 2 more tests and some docs, let me know if this looks okay

@xaviPastHubs
Copy link
Author

@jorenbroekema Yes, the tests you've added look great! Appreciate the hard work you put in

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