Skip to content

Conversation

gaperton
Copy link

@gaperton gaperton commented Nov 1, 2019

I like the way how this example works, an ability to load views dynamically is amazing feature of SDK 4.0. I made some touches to this example to address common design problems developer will experience when he will try to use it in a real app.

  • added a view stack with open() and back() methods, and automatic handling of the BACK button.
  • added the missing ability to clean up sensors and clock event handlers when the view is closed.
  • added the ability to pass options and global context to views.
  • refactored an API to have more clean and efficient views initialization.
  • added README.

Now it's basically a quite useful microframework and project starter, not just a feature example.

@Hexxeh
Copy link

Hexxeh commented Nov 16, 2019

This looks great, thanks for your contribution!

@Hexxeh Hexxeh self-requested a review November 16, 2019 22:16
let btn = document.getElementById("v1-button");
btn.addEventListener("click", clickHandler);
}
export function initialize( views, options ){
Copy link

Choose a reason for hiding this comment

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

Might be neater and less prone to typos to use the default export?

/**
* Open view-2 as subview and pass granularity as a parameter.
*/
btn3.addEventListener( "click", evt => {
Copy link

Choose a reason for hiding this comment

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

nit: Spacing on the arguments looks a bit odd on the event handlers here compared to other files

return () => {
// Unsubscribe from clock.
clock.granularity = "off";
clock.ontick = void 0;
Copy link

Choose a reason for hiding this comment

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

In an example, this particular syntax is likely to confuse some folks. Perhaps stick to just undefined?

viewSelected: () => viewSelected
};
/** Open the view as subview, so back button can be used to navigate back */
open( viewName, options ){
Copy link

Choose a reason for hiding this comment

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

nit: perhaps push would be a more descriptive name here? To me, open vs replace isn't super clear about the difference between the two.

@orviwan
Copy link
Collaborator

orviwan commented Nov 18, 2019

Sorry for the delay. I've just merged the DCO and licence. Can you sign your commit as per https://dev.fitbit.com/community/contributing/. Thanks for an awesome contribution!

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.

3 participants