Closed Bug 1192740 Opened 9 years ago Closed 9 years ago

Horizontal blue bar from Hello panel switches tab when adding new contact or editing one

Categories

(Hello (Loop) :: Client, defect, P1)

defect

Tracking

(firefox42 affected, firefox43 fixed)

RESOLVED FIXED
mozilla43
Iteration:
43.1 - Aug 24
Tracking Status
firefox42 --- affected
firefox43 --- fixed

People

(Reporter: bmaris, Assigned: dcritchley)

References

Details

(Keywords: regression, Whiteboard: [visual refresh bug])

User Story

Need
As a user I want the correct tab to be selected when on contact add page

Deliverable
Components highlight the correct tab header

Solution
Create a new component that encompasses the child components:
* Contact List
* Contact Add
* Contact Edit
* Contact Import

New parent component, "ContactsControllerView", utilizes state variable and switch case to render the current view and facilitate switching views. All methods utilized to switch views will be contained within "ContactsControllerView" and will be passed to the child components as props. Set initial state to be rendered to "Contact List" component. Function added to "ContactsControllerView" that child components can call to change view to specific state.

Remove "hide" logic from tab view

Attachments

(2 files, 3 obsolete files)

Affected builds:
- latest Nighty 42.0a1

Affected OS`s:
- Windows 7 64-bit
- Ubuntu 14.04 32-bit
- Mac OS X 10.10.4

STR:
1. Start Firefox
2. Click Hello icon
3. Click Get started
4. Click Hello icon again
5. Login with a FxA account
6. Click Add Contact

Expected results: Thin horizontal blue bar (below selected tab) sticks to Contacts tab, as shown in mockup: http://i.sevaan.com/image/0y350t1Y1230/o

Actual results: Blue bar switches bellow Conversations and both icons (contacts and conversations) are grey.

Notes:
- Gif attached showing the issue.
- This is not a recent regression, it reproduces after the implementation from bug 1183386.
will this get fixed via visual refresh or should i take this as a high priority bug
Flags: firefox-backlog+
Priority: -- → P1
Whiteboard: [visual refresh bug]
This should be fixed alongside the refresh.
Blocks: 1179193
Rank: 23
Keywords: regression
Assignee: nobody → david.critchley
User Story: (updated)
Comment on attachment 8649564 [details] [diff] [review]
Adding New contacts parent component for contact list, contact add, contact edit and contact import views under Contacts tab

Review of attachment 8649564 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good! Can you also add `ContactsControllerView` to the showcase in the edit mode so that we can have a look at that view as well?
I don't think you can add the whole panel in that state because you would have no way to pass in a contact but if I'm wrong even better!

::: browser/components/loop/content/js/contacts.jsx
@@ +524,5 @@
>        });
>      },
>  
>      handleAddContactButtonClick: function() {
> +      this.props.switchToContactAdd({});

nit: Could you do `|| {}` in handleAddEditContact instead of passing in an empty object?

@@ +803,5 @@
>    const ContactDetailsForm = React.createClass({
>      mixins: [React.addons.LinkedStateMixin],
>  
>      propTypes: {
> +      contactFormData: React.PropTypes.object,

Since you are always passing this prop down in the controller can you make it required?

@@ +853,1 @@
>        let contactsAPI = navigator.mozLoop.contacts;

Change to use mozLoop from props here as well.

::: browser/components/loop/test/desktop-local/contacts_test.js
@@ +307,5 @@
> +
> +    beforeEach(function() {
> +      view = TestUtils.renderIntoDocument(
> +        React.createElement(loop.contacts.ContactsControllerView, {
> +          /* ** */

This should be removed.

@@ +322,5 @@
> +    });
> +  });
> +
> +  /*
> +   To Check stub or Spy use:

Useful but this should be removed.

@@ +334,5 @@
> +
> +    beforeEach(function() {
> +      view = TestUtils.renderIntoDocument(
> +        React.createElement(loop.contacts.ContactsControllerView, {
> +          /* ** */

This should be removed.

@@ +354,5 @@
> +
> +    beforeEach(function() {
> +      view = TestUtils.renderIntoDocument(
> +        React.createElement(loop.contacts.ContactsControllerView, {
> +          /* ** */

This should be removed.

@@ +362,5 @@
> +        }));
> +    });
> +
> +    it("should switch component to Contact Add view", function() {
> +      view.handleAddEditContact("contactAdd")({});

If you make the change I suggested you could call this without the empty object.

@@ +373,5 @@
> +
> +      expect(view.refs.contacts_edit).to.not.eql(null);
> +    });
> +  });
> +

Extra blank line.

@@ +579,5 @@
>            sinon.assert.calledOnce(fakeWindow.close);
>          });
>      });
>  
> +

Extra blank line.

@@ +606,5 @@
> +        function() {
> +          listView.handleContactAction({}, "edit");
> +
> +          sinon.assert.calledOnce(listView.props.switchToContactEdit);
> +

Extra blank line.

@@ +773,5 @@
> +        it("should call switchToInitialView when Add Contact button is clicked", function() {
> +          var nameInput = view.getDOMNode().querySelector("input[type='text']");
> +          TestUtils.Simulate.change(nameInput, {target: {value: "Example"}});
> +          var emailInput = view.getDOMNode().querySelector("input[type='email']");
> +          TestUtils.Simulate.change(emailInput, {target: {value: "test@example.com"}});

Please group variable declarations, test setup and assertions together.

vars ...

setup (Simulate.click etc) ...

assertions ...

::: browser/components/loop/ui/ui-showcase.jsx
@@ +1653,5 @@
>    window.addEventListener("DOMContentLoaded", function() {
>      var uncaughtError;
>      var consoleWarn = console.warn;
>      var caughtWarnings = [];
> +    /*

This should be removed.
Attachment #8649564 - Flags: review?(andrei.br92) → review-
Comment on attachment 8650201 [details] [diff] [review]
Adding New contacts parent component for contact list, contact add, contact edit and contact import views under Contacts tab

Review of attachment 8650201 [details] [diff] [review]:
-----------------------------------------------------------------

Almost there! You need to rebase your patch because I was not able to apply and test.

::: browser/components/loop/content/js/contacts.jsx
@@ +750,5 @@
> +      notifications: React.PropTypes.object.isRequired
> +    },
> +
> +    getInitialState: function() {
> +      return {currentComponent: this.props.initialSelectedTabComponent || "contactList",

Move `currentComponent` on the next line and indent accordingly.

@@ +777,5 @@
> +    /* XXX Consider whether linkedStated makes sense for this */
> +    render: function() {
> +      switch(this.state.currentComponent) {
> +        case "contactAdd":
> +          return (<ContactDetailsForm contactFormData={this.state.contactFormData}

Move this prop on the next line and indent it once (2 spaces). Indent all props similarly for all 3 cases.

@@ +825,5 @@
>      },
>  
>      initForm: function(contact) {
>        let state = this.getInitialState();
> +      if (_.keys(contact).length > 0) {

Add a comment about testing for empty object.

::: browser/components/loop/test/desktop-local/contacts_test.js
@@ +590,5 @@
> +
> +          sinon.assert.calledOnce(listView.props.switchToContactAdd);
> +        });
> +
> +      it("should call switchToContactEdit function when Edit Contact button is clicked",

You are not doing a click in this test.

::: browser/components/loop/ui/ui-showcase.jsx
@@ +17,5 @@
>    // 1.1 Panel
>    var AvailabilityDropdown = loop.panel.AvailabilityDropdown;
>    var PanelView = loop.panel.PanelView;
>    var SignInRequestView = loop.panel.SignInRequestView;
> +  var ContactsControllerView = loop.contacts.ContactsControllerView;

You are not using this.
Attachment #8650201 - Flags: review?(andrei.br92) → review-
Adding New contacts parent component for contact list, contact add, contact edit and contact import views under Contacts tab
Attachment #8650201 - Attachment is obsolete: true
Attachment #8650617 - Flags: review?(andrei.br92)
Rank: 23 → 19
Comment on attachment 8652039 [details] [diff] [review]
Adding New contacts parent component for contact list, contact add, contact edit and contact import views under Contacts tab

Review of attachment 8652039 [details] [diff] [review]:
-----------------------------------------------------------------

Great!
Attachment #8652039 - Flags: review?(andrei.br92) → review+
https://hg.mozilla.org/mozilla-central/rev/4772e02f0b5a
Status: NEW → RESOLVED
Closed: 9 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla43
Iteration: --- → 43.1 - Aug 24
You need to log in before you can comment on or make changes to this bug.