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

Deep linking (closes #331) #338

Merged

Conversation

wilsonpage
Copy link
Collaborator

No description provided.

@wilsonpage wilsonpage force-pushed the 331-poi-deep-linking-android branch 3 times, most recently from fddf95e to 263c024 Compare November 16, 2016 10:41
@wilsonpage
Copy link
Collaborator Author

@arcturus r? (ping-pong sorry)

@@ -0,0 +1,6 @@

module.exports = {
INCOMPLETE: 'incomplete',
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

FETCHING

isFetching: false,
value: null,
},
items: {},
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

itemsCache?

@@ -2,8 +2,8 @@
* Dependencies
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Change filename to item-detail.js

@@ -0,0 +1,101 @@
'use strict';
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

-> item-container.js

@@ -5,8 +5,8 @@
*/
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

list-item.js

openItem: PropTypes.string,
items: PropTypes.object,
setOpenItem: PropTypes.func,
dispatch: PropTypes.func.isRequired,
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Maybe can remove some of these?

const items = state.itemsNearby
.map(url => resolveItem(url))
.filter(item => !!item);
function mapStateToProps({ items, itemsNearby, userFlags, indicateScanning }) {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Remove items

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ignore

@@ -35,20 +35,25 @@ class ApiMetadata: ApiBase {
//
// data["objects"] = elems
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Remove comment

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

*Update

@arcturus
Copy link
Member

arcturus commented Nov 16, 2016

Follow ups:

@arcturus
Copy link
Member

@wilsonpage after rebasing to latest master (it has a patch from @samgiles for react-native version) it does kind of work in iphone.

It at least tryies to load the view, it loads the basic, and then the app dies cause an exception in the new postArray method, need to debug more to understand whats going on.

@arcturus
Copy link
Member

More feedback about what's going on.

It's important to take into account the environment where I am, surronded by beacons that we constantly receive, so I have hundreds of calls to app render scene home in a matter of 10 seconds. Either that or we are constantly triggering model change, and also calling fetchItemIfNeeded even if it's going to end soon, we still have an enormous number of calls to that function.

That is making the animation to open the item to be very slow and to compite for resources. When we load an item that is in the cache everything seems to work ok.

@arcturus
Copy link
Member

Yes, I can confirm that in the simulator without so many update cause of btle beacons everything works way smoother.

We can consider this like an aggressive environment, but we should think on adding some throttling to that update coming from BLE.

@wilsonpage wilsonpage force-pushed the 331-poi-deep-linking-android branch from f672a67 to 0f723ff Compare November 19, 2016 01:29
@wilsonpage
Copy link
Collaborator Author

@arcturus:

  • Fixed perf issue caused by transitions being hammered by react re-render
  • Tested deep links working on real ios device
  • Ready to land

@arcturus
Copy link
Member

Shiiiiip it once travis is green :)

@wilsonpage wilsonpage merged commit 2d46b77 into mozilla-magnet:master Nov 21, 2016
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