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

Unblock React Routing #975

Closed
Abby-Wheelis opened this issue Sep 12, 2023 · 5 comments
Closed

Unblock React Routing #975

Abby-Wheelis opened this issue Sep 12, 2023 · 5 comments

Comments

@Abby-Wheelis
Copy link
Member

The three main screens are either re-written or almost re-written in React, and React Routing will enable smoother handling of things like the permissions popup, as well as facilitate re-writing the onboarding flow. Before we can convert to React Routing, all UI components need to be in React.

There were/are a few remaining components in ProfileSettings to be re-written. The log pages have been re-written in Profile changes to unblock routing, but ControlCollectionHelper and ControlSyncHelper have ionic/angular components and will need to be converted soon as well.

These helpers are stored outside of e-mission-phone in e-misssion-data-collection and cordova-server-sync respectively. This is the first part of the React re-write where we've needed to alter things outside of e-mission-phone, so it is worth taking time to consider the best course of action.

Some considerations for both options:
re-write "in-place"

  • keeps plugin-related code encapsulated with the plugin
  • javascript is mostly pass-through, custom code is in the UI elements

re-write into e-mission-phone

  • keeps UI code together, I don't know of any UI code that isn't in e-mission-phone
  • easy to access components like AlertBar to re-use
  • simple to connect to the rest of the Settings UI code
  • we already interface with plugins directly in the UI code in other places -- permissions, sensed data, logs, this does not seem much different
@Abby-Wheelis
Copy link
Member Author

I'm personally in favor of re-writing into e-mission phone, at least for the sake of simplicity. As I'm thinking about these pieces and looking at how they're used, I think there's an additional pro for moving the UI code from the helpers into e-mission-phone: there's a good bit of related code (at least 100 lines) that could be pulled out of ProfileSettings and kept in the new helper files. ProfileSettings is a large file right now, and pulling some of the logic for things like forceSync into the helper file could help clean up the main ProfileSettings code.

@JGreenlee
Copy link

I agree.

It would also simplify the build process. We wouldn't need extra scripts to download the files kept outside e-mission-phone.

@shankari
Copy link
Contributor

To me the argument that most resonates is that the settings popup is in e-mission-phone now.
Display the sensor values etc is something that other apps may want to skip - they don't have to have a developer zone.
But they do need to have all the permissions set up correctly for the plugins to work.
So I think that the settings and the permission screen are more intimately coupled with the plugin than the other functionality.

However, other than Transway, none of our partners are writing their own UI, and Transway has rewritten in native code so won't be using our javascript anyway. So it may make sense to leave everything in e-mission-phone for now, the one ask I would have is to make it somewhat easy to move back if this changes.

In particular, if we rewrite the plugins as react native plugins, we may find a larger community that wants to use them and may have different use cases that we have seen so far.

@Abby-Wheelis
Copy link
Member Author

For a second, I was confused how we could have these components be separate, and import UI components as well as just functions, but then I remembered that we can import components (and do all the time within different parts of e-mission-phone) so importing from an outside source would be no different. In terms of keeping it easy to move back, I'll try to achieve that by having one-way dependency, where the helpers don't depend on other parts of the repo, but other parts of the repo can depend on them. I can see where I've used AlertBar and ActionMenu (components defined in e-mission-phone) in the helpers, but might move away from these if possible.

@shankari
Copy link
Contributor

@Abby-Wheelis I believe this has been fixed in e-mission/e-mission-phone#1029. Please reopen if not

@github-project-automation github-project-automation bot moved this from Issues being worked on to Tasks completed in OpenPATH Tasks Overview Oct 21, 2023
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