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)
Hello (Loop)
Client
Tracking
(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)
5.88 KB,
image/png
|
Details | |
49.02 KB,
patch
|
andreio
:
review+
|
Details | Diff | Splinter Review |
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.
Reporter | ||
Updated•9 years ago
|
status-firefox42:
--- → affected
Comment 2•9 years ago
|
||
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]
Comment 3•9 years ago
|
||
This should be fixed alongside the refresh.
Assignee | ||
Updated•9 years ago
|
Assignee: nobody → david.critchley
Assignee | ||
Updated•9 years ago
|
User Story: (updated)
Assignee | ||
Comment 4•9 years ago
|
||
Attachment #8649564 -
Flags: review?(andrei.br92)
Comment 5•9 years ago
|
||
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-
Assignee | ||
Comment 6•9 years ago
|
||
Attachment #8649564 -
Attachment is obsolete: true
Attachment #8650201 -
Flags: review?(andrei.br92)
Comment 7•9 years ago
|
||
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-
Assignee | ||
Comment 8•9 years ago
|
||
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)
Updated•9 years ago
|
Rank: 23 → 19
Assignee | ||
Comment 9•9 years ago
|
||
Attachment #8650617 -
Attachment is obsolete: true
Attachment #8650617 -
Flags: review?(andrei.br92)
Attachment #8652039 -
Flags: review?(andrei.br92)
Comment 10•9 years ago
|
||
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+
Comment 12•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/4772e02f0b5a
Status: NEW → RESOLVED
Closed: 9 years ago
status-firefox43:
--- → fixed
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla43
Updated•9 years ago
|
Iteration: --- → 43.1 - Aug 24
You need to log in
before you can comment on or make changes to this bug.
Description
•