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(core): Add autoStart feature #98

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

Conversation

krarrobo1
Copy link
Contributor

@krarrobo1 krarrobo1 commented Apr 28, 2023

This PR address issue #50

@krarrobo1 krarrobo1 self-assigned this May 2, 2023
Copy link
Member

@JoseLion JoseLion left a comment

Choose a reason for hiding this comment

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

This is looking good so far! I left a few comments, but we need to know how this would work with multiple tours. Let me know if you want to pair on that 🙂

@@ -36,6 +36,8 @@
},
"dependencies": {
"fast-equals": "^5.0.1",
"object-hash": "^3.0.0",
"react-native-mmkv-storage": "^0.9.1",
Copy link
Member

Choose a reason for hiding this comment

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

Let's move this to devDependencies as it's an optional peer dependency 🙂

@@ -73,6 +76,7 @@
"peerDependencies": {
"react": ">=16.8.0",
"react-native": ">=0.50.0",
"react-native-mmkv-storage": ">=0.9.1",
Copy link
Member

Choose a reason for hiding this comment

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

We need to add this one to peerDependenciesMeta and mark it as optional: true

Comment on lines +3 to +5
const storage = new MMKVLoader().initialize();

export default storage;
Copy link
Member

Choose a reason for hiding this comment

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

We should try to avoid default exports in favor of named exports 🙂

Suggested change
const storage = new MMKVLoader().initialize();
export default storage;
export const storage = new MMKVLoader().initialize();

@@ -0,0 +1,5 @@
import { MMKVLoader } from "react-native-mmkv-storage";
Copy link
Member

Choose a reason for hiding this comment

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

This is an optional dependency, so it's possible that it's not present in some environments. Shouldn't the import fail in those cases? A way to solve that is by using dynamic imports.

import("react-native-mmkv-storage")
  .then(({ MMKVLoader }) => {
    // do something with the imported modules
  })
  .catch((error: unknown) => {
    // handle whenever the module is not present
  });

@@ -0,0 +1,5 @@
import { MMKVLoader } from "react-native-mmkv-storage";

const storage = new MMKVLoader().initialize();
Copy link
Member

Choose a reason for hiding this comment

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

Maybe we should initialize the store only when the autoStart option is set to always | once, otherwise it makes no sense to initialize something that is not going to be used 😉

} = props;

const [current, setCurrent] = useState<number>();
const [spot, setSpot] = useState(ZERO_SPOT);
const [tourId, setTourId] = useMMKVStorage("tourId", storage, "");
Copy link
Member

Choose a reason for hiding this comment

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

Maybe we don't need to use the hook here. We don't need to use the tourId as a state. We only need to get/set it upon the tour start call

Comment on lines +156 to +157
renderStep(0);
onStart?.();
Copy link
Member

Choose a reason for hiding this comment

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

I think these two can be replaced by calling the start() function


const startOnce = useCallback(() => {
if (!tourId) {
setTourId(hash(steps));
Copy link
Member

Choose a reason for hiding this comment

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

What happens if the user has multiple tour providers in their app. I think we will need them to provide an ID of the tour to use this feature, and the value we check against is the hash of the steps. What do you think? 🤔

package/src/lib/SpotlightTour.provider.tsx Show resolved Hide resolved
@@ -136,7 +138,18 @@ jest
timing: timingMock,
},
};
});
})
/* eslint-disable @typescript-eslint/no-unused-vars */
Copy link
Member

Choose a reason for hiding this comment

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

Why do we need this? 🤔

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