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

Refactoring proposal #69

Open
Ldoppea opened this issue Jan 17, 2022 · 11 comments
Open

Refactoring proposal #69

Ldoppea opened this issue Jan 17, 2022 · 11 comments

Comments

@Ldoppea
Copy link
Member

Ldoppea commented Jan 17, 2022

As the app is still in a draft state the code evolved a lot in the past months. Due to that, the architecture may not be ideal anymore and files may not be named/structured correcly.

I propose to use this thread to talk about refactoring.

Feel free to propose new refactoring so we can discuss about them.

@Ldoppea
Copy link
Member Author

Ldoppea commented Jan 17, 2022

First I would like to propose new names for components:

  • component screens/connectors/index.jsx -> Konnetors
    • this screen is the app squeleton. It contains: the HomeView, the connector launcher view and a debug view (dedicated to connectors).
    • New name proposal: screens/main/MainView.jsx
  • component screens/connectors/HomeView.jsx
    • New name proposal: screens/home/HomeView.jsx
  • component screens/Authenticate.js
    • New name proposal: screens/login/LoginView.jsx
  • component screens/CozyWebView.jsx
    • I do not consider it as a screen
      • HomeView is the screen that embed the CozyWebView component (same for StoreView)
    • New name proposal: screens/components/CozyWebView.jsx
  • component screens/routes/CozyAppView.jsx
    • New name proposal: screens/cozy-apps/CozyAppView.jsx
  • component screens/providers/SplashSCreenProvider.jsx
    • This is not a screen, we should have a folder dedicated to providers at the src's root
    • New name proposal: providers/SplashSCreenProvider.jsx
  • folder libs
    • This folder seems to mix ReactNative configuration code with connectors code etc
    • I don't know this code enough yet to propose a better solution, so let's discuss about this

(more to come later)

@Ldoppea
Copy link
Member Author

Ldoppea commented Jan 17, 2022

useAuthenticate take a setClient argument that has to be drilled from the main view.

Would it be easier to create a ClientContext on the app's root and use it instead?

But I'm also wondering if there is a need for that hook as it is used only on the Authenticate.hsx component which is pretty empty. Do we expect that hook to be used somewhere else in the future?

@Crash--
Copy link
Contributor

Crash-- commented Jan 18, 2022

screens should only be used for "screen page", aka the all screen.

So to me, we can't have a ./screens/main/MainView that display a ./screens/home/HomeView. Screens can not contain any other screen.

So:

  • ./screens/home/HomeScreen.jsx
  • ./screens/apps/CozyAppWebviewScreen.jsx

New name proposal: screens/components/CozyWebView.jsx

No need to put it in screens folder. Inside a component folder is enough. Maybe we can have ./components/webviews/CozyWebView.jsx (I'm guessing we'll have several webview components at the end)

New name proposal: providers/SplashSCreenProvider.jsx

👍

New name proposal: screens/home/HomeView.jsx

Let's only keep the screen at the root directory, why not screens/home/components/HomeView.jsx ?

StoreView has to be removed.

useAuthenticate take a setClient argument that has to be drilled from the main view.

We don't need all this stuff. CozyClient already handle all of that...

const client = new CozyClient({}}
if(client.isLogged){
...
} else {
...
}

And no need to use setClient or else, with useClient or withClient we already have mechanism to get the client....

@Crash--
Copy link
Contributor

Crash-- commented Jan 19, 2022

At the moment it's hard to understand how webview works:

Webviews contained in LaunchView are out of scope for now.

We should have one CozyWebView which is responsible to:

  • inject the fact that we're in the flagship app
  • handle the redirection (login / store / else...)
  • handle the messaging

CozyAppView should be removed.

After that we should only have Views that use CozyWebView. If needed those Views can add stuff the CozyWebView like setting the uri or else

@Ldoppea
Copy link
Member Author

Ldoppea commented Jan 19, 2022

New name proposal: screens/components/CozyWebView.jsx

No need to put it in screens folder. Inside a component folder is enough. Maybe we can have ./components/webviews/CozyWebView.jsx (I'm guessing we'll have several webview components at the end)

Woups, this was a typo, I had in mind to put it in the root src too 😅

Let's only keep the screen at the root directory, why not screens/home/components/HomeView.jsx ?

Do you mean that we have HomeScreen and components/HomeView? I see only one file for now, why should we have it split in 2 files?

@Ldoppea
Copy link
Member Author

Ldoppea commented Jan 19, 2022

the moment it's hard to understand how webview works:

My understanding is that CozyAppView will not be used anymore soon when we will homogenise how the store is handled. So this will fit your conclusion.

@Ldoppea
Copy link
Member Author

Ldoppea commented Jan 19, 2022

Do you mean that we have HomeScreen and components/HomeView? I see only one file for now, why should we have it split in 2 files?

I just got what you meant:

  • screens/connectors/index.js -> screens/home/HomeScreen.jsx
  • screens/connectors/HomeView.js -> screens/home/components/HomeView.jsx
    Is that correct?

@Ldoppea
Copy link
Member Author

Ldoppea commented Jan 19, 2022

I made a first batch of edits here : #72

Still has to be done (selection from previous posts):

  • folder libs
    • This folder seems to mix ReactNative configuration code with connectors code etc
    • I don't know this code enough yet to propose a better solution, so let's discuss about this
  • Move src/screens/connectors/LauncherView and src/screens/connectors/DebugView somewhere else
    • I realized that those components are not screens as they are embeded inside of HomeScreen
    • I don't know yet where to put them
  • Refactor LoginScreen.jsx (previously Authenticate.jsx)
    • Use cozy-client hooks instead of setClient
    • Use a CozyWebView instead of the raw react-native one

@Ldoppea
Copy link
Member Author

Ldoppea commented Feb 22, 2022

HomeView may need some refactoring as stated in this comment: #82 (comment)

@acezard
Copy link
Contributor

acezard commented Feb 7, 2023

@Ldoppea do you think this issue is still relevant?

@Crash--
Copy link
Contributor

Crash-- commented Jun 22, 2023

We should close, no?

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

No branches or pull requests

3 participants