916826 - [FTE] Refactor navigation code

Status: RESOLVED WONTFIX

(Firefox OS Graveyard :: Gaia::First Time Experience, defect)

Description

[Fernando Campo (:fcampo)]

After changes introduced in bug 909262, navigation.js code is getting a little messy. 

Is highly recommended to apply changes to simplify the flow and beautify the current code.

Comment 1

[Marina Rodríguez [:mai]]

Attachment: proposal patch v1

Please, could you review the code? I think it's easier than the actual solution.

Comment 2

[Marina Rodríguez [:mai]]

Attachment: proposal patch v1

Comment 3

[Borja Salguero [:borjasalguero]]

Comment on attachment 809109 [details]
proposal patch v1

In this case, I would move to a real 'refactor' (currently the patch only is fixing small details from https://github.com/mozilla-b2g/gaia/pull/11953/files). First of all I would like to have a navigation file which will be agnostic about the steps (currently we have a lot of code checking if step.hash === '#value'), so I would move to a new concept, where the list of steps and options is loaded (actually even we could have several step design for Device & OEM - Imagine that we want to change the flow for the tablet for example-).
Other issue that I'm really concerned is about the 'innerHTML' use for l10n.
So IMHO we should create an object called 'Step', with all the info related with this step (conditions to be shown, l10n header...), and Navigation will handle with a 'collection/list' of steps.

Comment 4

[Jason Smith [:jsmith]]

Not blocking - refactorings don't block. Ask for approval.

Comment 5

[Fernando Campo (:fcampo)]

Recent merge of bug 996207 will give us a little help for the navigation steps

Comment 6

[Guillaume Marty [:gmarty]]

Unassigning me as it's very unlikely that I'll work on this anytime soon.

Comment 7

[Giovanni Charles]

I've come up with an idea for the refactor. I will start working on it since my time is limited but I'm open to criticism.

I would like to separate the spaghetti into 2 different problems:
1. The flow logic. Deciding which step to go to next.

When the user wants to change step Navigation.forward/Navigation.back will iterate through the steps until it finds a step that can be navigated to. When it does, it will change the window hash to that step's hash.

How does Navigation know to skip a step?

I hope to change the steps list to have a requirements object that looks something like this:
{
  sim: 1, 
  network: { wifi: true }, 
  viewOnce: true, 
  version: { min: 1.3, max: 2.0 }
}

And add a mapping to policy enforcers like this:
{
  'sim': function(criteria) {
    var shouldSkip;
    //sim management code here
    return shouldSkip;
  },
  'network': function(criteria) {
    var shouldSkip;
    //network management code here
    return shouldSkip;
  },
  ...
}

So that Navigation can iterate through the fields of the requirements(if any) and check with the enforcers for any reason to skip the step.

This should remove Navigation.skipStep which I don't think anyone likes and make the flow clearer. Thank you Borja for the inspiration.

2. The step setup. Given that we know which step we are going to we want to the following:
*teardown the last step
e.g. adding a back button after leaving the first step, removing the refresh button after leaving the wifi page...

*perform generic step setup (which I think will become Navigation.ManageStep)
ONLY code that applies to every step e.g. change the header title...

*perform step specific setup
e.g. adding the refresh button and populating networks when landing on the wifi step, changing navigation buttons... 
While this is being done, localisation should be fixed so that it is not being called on every step change and instead using 'data-l10n-id'

I want to put ALL of this in the event handler for the window hash change so that it is even more separated from the flow. Jumping to a step from anywhere should do all the setup you expect.

For this I hope to:
- Pull the common functionality into Navigaition.manageStep. By adding a title to the step object, as proposed by Borja.
- Then add anything else into teardown and setup functions for the step object.

This should get rid of quite a few switch statements, repeated code and allow us to add/change steps simply by modifying Navigation's step list

Comment 8

[Sam Foster [:sfoster] (he/him)]

(In reply to Giovanni Charles from comment #7)
> I've come up with an idea for the refactor. I will start working on it since
> my time is limited but I'm open to criticism.

Sounds like the right place to start, I'm looking forward to seeing whatever you can come up with. I know you're low on time - I'll assume ownership of this when you are out (though maybe fcampo wants to pick it up?), please just attach a work-in-progress patch or branch to the bug when you have something, and be clear about the status of the work - what is done, not done, what parts of the implementation are speculative/experimental. And let us know when you are stopping work on it - Reset Assignee to default or assign to me.

Comment 9

[Giovanni Charles]

Will do

Comment 10

[Giovanni Charles]

I have been swamped with administration and trip planning so this hasn't happened, see comments in Bug 1067677. I'll leave this open to anyone who dares do it.

Comment 11

[BMO Automation]

Firefox OS is not being worked on