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

[No QA] Clarify Onyx method usage in View #5999

Merged
merged 3 commits into from
Oct 23, 2021
Merged

Conversation

marcaaron
Copy link
Contributor

@marcaaron marcaaron commented Oct 22, 2021

Details

There are not too many usages here, but I cleaned them up by creating action methods.

Fixed Issues

$ #5998

Tests / QA Steps

All of these changes should be syntax and shouldn't have any bearing on the app behavior.
Therefore, we just need to run regular regressions.

Tested On

  • Web
  • Mobile Web
  • Desktop
  • iOS
  • Android

@marcaaron marcaaron self-assigned this Oct 22, 2021
@marcaaron marcaaron marked this pull request as ready for review October 22, 2021 23:18
@marcaaron marcaaron requested a review from a team as a code owner October 22, 2021 23:18
@MelvinBot MelvinBot requested review from thienlnam and removed request for a team October 22, 2021 23:18
thienlnam
thienlnam previously approved these changes Oct 22, 2021
README.md Outdated
Comment on lines 156 to 157
**Note:** As a convention, the UI layer should never interact with device storage directly or call `Onyx.set()` or `Onyx.merge()`. Use an action!

Copy link
Contributor

Choose a reason for hiding this comment

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

NAB: Linking to an example where we use an action instead of calling Onyx.merge/set

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added an example with some permalinks and more explanation into the "why" (at least my interpretation of it). Lmk if it makes sense, sounds reasonable, etc 😄

Copy link
Contributor

@thienlnam thienlnam left a comment

Choose a reason for hiding this comment

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

Looks great, thanks!

@marcaaron marcaaron merged commit 2a2e791 into main Oct 23, 2021
@marcaaron marcaaron deleted the marcaaron-onyxUsagesInViews branch October 23, 2021 00:46
@OSBotify
Copy link
Contributor

🚀 Deployed to staging by @marcaaron in version: 1.1.8-12 🚀

platform result
🤖 android 🤖 success ✅
🖥 desktop 🖥 success ✅
🍎 iOS 🍎 success ✅
🕸 web 🕸 success ✅

@roryabraham roryabraham changed the title Clarify Onyx method usage in View [No QA] Clarify Onyx method usage in View Oct 25, 2021
@OSBotify
Copy link
Contributor

🚀 Deployed to production by @roryabraham in version: 1.1.10-2 🚀

platform result
🤖 android 🤖 success ✅
🖥 desktop 🖥 success ✅
🍎 iOS 🍎 success ✅
🕸 web 🕸 success ✅

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.

None yet

3 participants