Closed Bug 976535 Opened 6 years ago Closed 6 years ago

[FTE] Refactor tutorial

Categories

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

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

(b2g-v2.0 fixed)

RESOLVED FIXED
2.0 S1 (9may)
Tracking Status
b2g-v2.0 --- fixed

People

(Reporter: borjasalguero, Assigned: borjasalguero)

References

Details

Attachments

(1 file)

46 bytes, text/x-github-pull-request
eragonj
: review+
fcampo
: review+
fcampo
: feedback+
eragonj
: feedback+
Details | Review
Currently tutorial is handled by JS, and we can simplify this with CSS only. Removing this navigation here is the first step to reduce the complexity as well here https://bugzilla.mozilla.org/show_bug.cgi?id=916826.
Assignee: nobody → borja.bugzilla
Attached file Pull request
Attachment #8382128 - Flags: feedback?(ejchen)
Attachment #8382128 - Flags: feedback?(francisco.jordano)
Blocks: 916826
This is the fist step for testing a simple navigation, where we reduce the JS logic (for example for the progress bar) where it's not needed.

We are supposed to be working in a lot of new devices (new tablets, resolutions...), so dealing with CSS  & responsive design is a must here.
Attachment #8382128 - Flags: feedback?(francisco.jordano) → feedback?(fernando.campo)
Comment on attachment 8382128 [details] [review]
Pull request

Left some comments on Github, Borja ! Please check it ! 

Nice work, by the way. :P
Attachment #8382128 - Flags: feedback?(ejchen)
Comment on attachment 8382128 [details] [review]
Pull request

Updating the flag properly as all comments were addressed yesterday. Thanks for all the comments!
Attachment #8382128 - Flags: feedback?(ejchen)
Comment on attachment 8382128 [details] [review]
Pull request

Nice work Borja !! These changes are awesome !!! :P
Attachment #8382128 - Flags: feedback?(ejchen) → feedback+
Comment on attachment 8382128 [details] [review]
Pull request

Sorry it took me long, honestly I thought I've had given the f+ lat week :S

Anyway, I like the way it's handled, and as we talked offline, I would like to apply this approach to the rest of the FTU (that needs a good refactor so badly).

I left couple of comments on github anyway, to make code a little more clear.

Great job Borja!
Attachment #8382128 - Flags: feedback?(fernando.campo) → feedback+
Blocks: 902487
Hi guys.
Can we land this patch? What we need is probably a bit of merging when bug 984404 lands (today or tomorrow) and then update the tests.

Borja, do you have time to do that? If not, I can do it.

The reason I want this quickly is because I've started working on refactoring the navigation, as per bug 984404 and bug 916826.
Depends on: 984404
Hi Guillaume! Im gonna try to update this today.

On the other hand I've seen you are interested in working on the refactor of the Navigation of FTU, which is a really good idea. This bug was discussed previously with Eragon, trying to get a common idea about how the Navigation object should look like (take a look to bug https://bugzilla.mozilla.org/show_bug.cgi?id=927807). Which is your final idea about the refactor of Navigation? Please feel free to assign to you the bug https://bugzilla.mozilla.org/show_bug.cgi?id=916826, and dont hesitate to ask! Definitely this is a great win, and I'm more than happy if you can work on it (and help you if needed as well). Thanks!
Flags: needinfo?(gmarty)
Thanks, that would be great to get this PR updated and landed!

I'm pretty new to the code base, so I'd appreciate if you can give me some help to get me started in the right direction. When are you free to discuss this?
Flags: needinfo?(gmarty)
Comment on attachment 8382128 [details] [review]
Pull request

Ready to review! Now with tests & rebased.
Attachment #8382128 - Attachment description: WIP Patch → Pull request
Attachment #8382128 - Flags: review?(ejchen)
Attachment #8382128 - Flags: review?(fernando.campo)
(In reply to gmarty from comment #9)
> Thanks, that would be great to get this PR updated and landed!
> 
> I'm pretty new to the code base, so I'd appreciate if you can give me some
> help to get me started in the right direction. When are you free to discuss
> this?

Guillaume, I've just assigned to you the bug https://bugzilla.mozilla.org/show_bug.cgi?id=916826, so we can talk directly there. Please use 'needinfo' flag when you need my review,feedback or help there!

On the other hand, it would be great if you can describe your idea about the refactor in the bug 916826, so we will have a clear picture of the things needed to implement, and the discussion & suggestions added. Wdyt?

Thanks again and welcome to FTU! :)
Flags: needinfo?(gmarty)
Comment on attachment 8382128 [details] [review]
Pull request

Nice work Borja +++++++

I left some comments on the bug and please check it when you have time !

Bypass the r? to Fernando :)
Attachment #8382128 - Flags: review?(ejchen) → review+
Flags: needinfo?(gmarty)
The PR looks good, Borja can you address the few comments by eragonj? Then we can merge and I can start working on the incremental refactoring we discussed last week.
Just waiting to have access to a tablet to test the PR. 
Code looks good and works nice on Nightly, but I want to be sure we don't break anything
Updating right now! :)
Comment on attachment 8382128 [details] [review]
Pull request

tablet seal of approval :)
Attachment #8382128 - Flags: review?(fernando.campo) → review+
Cool! That means that we can merge it? :-)
Blocks: 949439
Target Milestone: --- → 2.0 S1 (9may)
You need to log in before you can comment on or make changes to this bug.