Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

33 details dropdown #87

Merged
merged 28 commits into from Feb 27, 2019

Conversation

@meandavejustice
Copy link
Contributor

commented Feb 25, 2019

Fixes #33

firefox_kxxdcofv2o
firefox_61wppgfm92

6a68 and others added some commits Dec 14, 2018

WIP: so close, got nothin
When webdriver opens the extension page, the browser.experiments.logins
API is not present. When I open the extension page myself, via going to
about:debugging, and doing
  browser.tabs.create({
    url: browser.extension.getURL(
           "/test/integration/test-pages/logins-api.html")
  }),
the test page seems just fine. Maybe webdriver needs some magic powers?
Final tweaks to update() API method
* The webextensions framework auto-inserts 'null' default values for any
specified keys that are missing, causing updates to break
LoginManager.modifyLogin() type checking. Filter out those null values.

* Create a coding convention to make clearer which things are vanilla JS
objects with login fields, which are nsILoginInfo objects, and which are
nsIPropertyBags used to modifyLogin.
Merge pull request #62 from lmorchard/21-logins-api-hookup
Initial minimal logins API connection to datastore

@meandavejustice meandavejustice requested a review from lmorchard Feb 25, 2019

@meandavejustice meandavejustice requested a review from mozilla-lockwise/desktop-engineering as a code owner Feb 25, 2019

@ghost ghost assigned meandavejustice Feb 25, 2019

@ghost ghost added the in progress label Feb 25, 2019

@meandavejustice

This comment has been minimized.

Copy link
Contributor Author

commented Feb 25, 2019

No idea why commit history is looking so insane, it's been rebased with master and the diffs are correct

@linuxwolf
Copy link
Member

left a comment

Overall it's looking great!

I have some inline suggestions, and some global things to address:

  • The unit- and integration-tests are failing
  • The hit target on the "‹" (go back) is very tiny now

The "go back" hit target on master:
lockbox-addon master-back-hit-target

And that hit target on 33-details-dropdown:
lockbox-addon 33-back-hit-target

src/list/components/item-fields.js Outdated Show resolved Hide resolved
src/list/components/item-fields.js Outdated Show resolved Hide resolved
src/list/components/item-fields.js Outdated Show resolved Hide resolved
@lmorchard
Copy link
Contributor

left a comment

Took a swing through and found a couple nits. We've also got some bits that are going to conflict between here and PR #90. But, the changes here are more involved (e.g. two buttons in a toolbar versus my one added delete button) - so I'd be inclined to let this land first and then revise / rebase my PR

src/background/datastore.js Outdated Show resolved Hide resolved
src/list/components/item-fields.js Outdated Show resolved Hide resolved

@linuxwolf linuxwolf self-requested a review Feb 27, 2019

@meandavejustice

This comment has been minimized.

Copy link
Contributor Author

commented Feb 27, 2019

@linuxwolf Addressed hit target on back button 👍

@linuxwolf
Copy link
Member

left a comment

really looking and feeling awesome!

Some things I really should have caught sooner; very sorry for not!

src/list/components/item-fields.js Show resolved Hide resolved
src/list/components/item-fields.js Outdated Show resolved Hide resolved
src/list/components/item-fields.js Outdated Show resolved Hide resolved

@meandavejustice meandavejustice requested a review from linuxwolf Feb 27, 2019

@linuxwolf
Copy link
Member

left a comment

r+

Perfect! :shipit:

@meandavejustice meandavejustice merged commit 763f7dc into mozilla-lockwise:master Feb 27, 2019

4 checks passed

WIP Ready for review
Details
codecov/patch 85.7% of diff hit (target 50%)
Details
codecov/project 95.2% (-0.5%) compared to 35fe96f
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@ghost ghost removed the in progress label Feb 27, 2019

@meandavejustice meandavejustice deleted the meandavejustice:33-details-dropdown branch Feb 27, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.