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

"New" button for all users & global state management discussion #791

Closed
rahulkgupta opened this issue Aug 26, 2024 · 6 comments · Fixed by #823
Closed

"New" button for all users & global state management discussion #791

rahulkgupta opened this issue Aug 26, 2024 · 6 comments · Fixed by #823
Assignees

Comments

@rahulkgupta
Copy link
Collaborator

rahulkgupta commented Aug 26, 2024

  • open up "add layer" panel with streets layer
  • make the button available to non pro users
@rahulkgupta rahulkgupta converted this from a draft issue Aug 26, 2024
@kfarr kfarr changed the title Update new functionality "new" available to all users, show "add layer" upon new Aug 31, 2024
@kfarr
Copy link
Collaborator

kfarr commented Sep 1, 2024

I made new button available to all users here: #823

I looked at triggering add layer panel but it was more difficult than expected because the state of whether or not the add layer panel is open is in another component (main component?) and not the toolbar component. Maybe revisit after redux?

@kfarr kfarr moved this from To Do - High Prio to For Review in 3DStreet Dev Tracking Sep 1, 2024
@vincentfretin
Copy link
Collaborator

Yes you have a state isAddLayerPanelOpen in the Main component.
The New button is indeed far away from Main component, it's Main>SceneGraph>ToolbarWrapper>Toolbar.
You can define a callback const setAddLayerPanelOpen = (open) => { this.setState({isAddLayerPanelOpen: open}); } that you pass as prop in SceneGraph, then in ToolbarWrapper then Toolbar, that's not ideal for maintenance and is called prop drilling. What is better is creating a context, and use the context in Toolbar. This is what we did for Geo or Auth context.
https://react.dev/learn/passing-data-deeply-with-context

It's been years since I used redux (that was in 2015), I had very bad experience with how we used it in a project, too much boilerplate, creating an action for every field in a form, rendering perf issues. This was before https://redux-toolkit.js.org/ @reduxjs/toolkit that simplifies the redux api so you don't write so much boilerplate anymore apparently. You still use react-redux package for integration with react. Note that React now have a good context api with useContext https://react.dev/reference/react/useContext and useReducer https://react.dev/reference/react/useReducer that was not the case in 2015, so you may not have a need for an additional library like redux if you don't need the store outside react.

About the trends, I see the react community on X and YouTube uses solutions like react-query, zustand or jotai for their global state management. zustand https://zustand-demo.pmnd.rs/ is similar to redux in the concepts, it started to be above @reduxjs/toolkit in the usage.
https://npmtrends.com/@reduxjs/toolkit-vs-jotai-vs-mobx-vs-react-redux-vs-redux-vs-zustand

I personally used jotai https://jotai.org in a project, it integrates well with React's context and hooks, that's a bit similar to Solid signals.
For some states like the microphone muted state because I need to access it from react and also in some event listener outside of react. In react code, it's a bit like a context and useState, not prop drilling.

Here is an example:

in atom.js

import { atom, createStore } from "jotai";
export const jotaiStore = createStore();

export const micEnabledAtom = atom(true);

export const getMicEnabled = (): boolean => {
  return jotaiStore.get(micEnabledAtom);
};

export const setMicEnabled = (enabled: boolean) => {
  jotaiStore.set(micEnabledAtom, enabled);
};

in index.js

import { Provider as JotaiProvider } from "jotai";
<JotaiProvider store={jotaiStore}>
  <App />
</JotaiProvider>

in MicButton.js

export const MicButton = () => {
  const [micEnabled, setMicEnabled] = useAtom(micEnabledAtom);
  ...
}

If you don't need the setter, simply:

const isBotEnabled = useAtomValue(isBotEnabledAtom);

in some callback:

cameraRig.setAttribute("player-info", {
  muted: !getMicEnabled(),
  name: name,
});

@vincentfretin
Copy link
Collaborator

Also note that we are using in this project for example Events.emit('opengeomodal');

const GeoPanel = () => {
const { currentUser } = useAuthContext();
const onClick = () => {
posthog.capture('geo_panel_clicked');
if (!currentUser) {
Events.emit('opensigninmodal');
} else if (currentUser.isPro) {
Events.emit('opengeomodal');
} else {
Events.emit('openpaymentmodal');
}
};

as a way to access the state setter, registering a listener just for that
Events.on('opengeomodal', () => {
posthog.capture('geo_modal_opened');
this.setState({ isGeoModalOpened: true });
});

What would be more familiar to a react developer is defining a setter

setGeoModalOpened = (open) => {
  posthog.capture('geo_modal_opened'); 
  this.setState({isGeoModalOpened: true});
}

and pass setGeoModalOpened as prop to GeoModal component.

With jotai, that could be rewritten like this:

in GeoModal.js:

const setGeoModalOpened = useSetAtom(geoModalOpenedAtom)
const onClick = () => {
  setOpenGeoModal(true);
};

in Main.js:

const isGeoModalOpened = useAtomValue(geoModalOpenedAtom)

in atom.js:

export const geoModalOpenedAtom = atom(false)

@rahulkgupta
Copy link
Collaborator Author

im open to using zustand, it seems like it's a popular library with decent integrations. Regardless of choice, global state management would be really good. So, if we're in agreement w/ zustand, let's go ahead and start adding it into the project!

@kfarr kfarr changed the title "new" available to all users, show "add layer" upon new show "add layer" upon new scene Sep 3, 2024
@kfarr kfarr changed the title show "add layer" upon new scene "New" button for all users & global state management discussion Sep 3, 2024
@kfarr kfarr self-assigned this Sep 3, 2024
@kfarr kfarr linked a pull request Sep 3, 2024 that will close this issue
@kfarr
Copy link
Collaborator

kfarr commented Sep 3, 2024

@vincentfretin
Copy link
Collaborator

I'm okay with zustand with the simplest form. I just want to avoid the reducers and actions, I have PTSD with those. :'D
All the open state for modals and panels are good candidates to include in the zustand store, this avoids lots of boilerplate of creating contexts.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging a pull request may close this issue.

3 participants