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

Playground screen UI states #33

Merged
merged 3 commits into from
Mar 20, 2023
Merged

Conversation

JozefCeluch
Copy link
Contributor

@JozefCeluch JozefCeluch commented Mar 13, 2023

Short Description

Adds UI states to the playground screen

What changes in the code?

The main goal was to learn how to define custom stores and data types with mobx-state-tree library.

Now there is a loading state that is followed by a content with auto-incremented number. When the number reaches 30 and error is shown. There is also a button to reset the counter. This should give us enough of a base to start adding some real code that fetches some data from somewhere and displays it on the screen.

Did you test?

manually only, no idea how to write tests here yet

paired with

@Frankie-Boy at the beginning where nothing made sense

.model('PlaygroundUiState', {
loading: types.maybe(Loading),
error: types.maybe(Error),
content: types.maybe(Content)
Copy link
Contributor

Choose a reason for hiding this comment

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

When would we use types.maybe() vs. types.optional()? (line 41)

Copy link
Contributor Author

@JozefCeluch JozefCeluch Mar 20, 2023

Choose a reason for hiding this comment

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

If I understand correctly, optional means that if the value is not there (i.e. persisted from last session) then it would set the default value provided as the second argument. Maybe seems like the actual nullable type (with two variants, one uses null and the other undefined to express missing value)

@@ -1,76 +1,108 @@
import React, { FC } from "react"
import * as React from "react"
Copy link
Contributor

Choose a reason for hiding this comment

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

This is fine but since we have allowSyntheticDefaultImports: true in tsconfig.json we don't need this change.

I actually found that this wildcard import is best practise (they exist in RN?) 😱 and shared another interesting guidelines below
facebook/react#18102
https://github.com/facebook/react-native-website/blob/main/STYLEGUIDE.md

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There's a difference in behaviour (at least on my VSCode so it might be my config) with autocomplete. When I start writing a function from react such as useEffect then autocomplete behaves differently depending on the import type:

In case of import * as React ... autocomplete will update the function call to React.useEffect
In case of import React ... the autocomplete will add another import {useEffect} and not change the function call

The import * is definitely more verbose but at least for now it makes it easier for me to understand where the code comes from even if it is probably not the correct JS approach

import {
Text, Screen
} from "../components"
import { isRTL } from "../i18n"
Copy link
Contributor

Choose a reason for hiding this comment

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

This is fine to go as its not really used, but incase it comes up again - this returns if the current device language direction is Right-To-Left. This is part of the I18nManager localization lib, which supports multiple languages including Arabic.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it's weird that this would need to be in the feature code though

}
<View style={[$bottomContainer, insets]}>
<Text tx="playgroundScreen.exciting" size="md" />
<Text text={`Value is: ${content.number}`} size="md" />
Copy link
Contributor

Choose a reason for hiding this comment

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

@juankysoriano had a question in the nav PR I didn't get to - but this shows off the difference between tx=screen.stringName and text={String Literal}. Hope this helps

}
function StateComponent(insets, uiState, onResetClicked) {
const { loading, error, content } = uiState
if (loading) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we make this a Switch Case operator?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure how to write a switch like that, these are separate properties of the uiState object where each can be either there or undefined

const $bottomContainerInsets = useSafeAreaInsetsStyle(["bottom"])
export const PlaygroundScreen: React.FC<PlaygroundScreenProps> = observer((props) => {
const $bottomContainerInsets = useSafeAreaInsetsStyle(["bottom"])
const { playgroundStore } = useStores()
Copy link
Contributor

Choose a reason for hiding this comment

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

🎉 Nice work

Leaving some extra info for others: https://mobx.js.org/defining-data-stores.html
The main responsibility of stores is to move logic and state out of your components into a standalone testable unit that can be used in both frontend and backend JavaScript.

Copy link
Contributor

@Frankie-Boy Frankie-Boy left a comment

Choose a reason for hiding this comment

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

Left a couple optional comments, LGTM

Did you test?
manually only, no idea how to write tests here yet

yet 😈 Looks like React Native comes with Jest for testing by default, which I've noted and we'll explore together https://jestjs.io/

@JozefCeluch
Copy link
Contributor Author

we can change/refactor code to address any other comments in next PRs

@JozefCeluch JozefCeluch merged commit a217be3 into development Mar 20, 2023
@JozefCeluch JozefCeluch deleted the playground_lce_states branch March 20, 2023 10:05
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.

2 participants