Doorhanger list view updates #64
Conversation
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?
* 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.
Logins API
Connected to #21
…API to apply changes
Initial minimal logins API connection to datastore
ghost
added
the
|
@meandavejustice looks like this needs a rebase and a few linter fixes |
There was a problem hiding this comment.
OK, taking a first pass:
Doorhanger default list view (spec)
WIP on the left, spec on the right:
This looks pretty good! Some visual stuff that looks off:
- The blue underscore shouldn't be there, but for accessibility, we need some other kind of focus indicator (maybe this is elsewhere in the spec, but if not, darkening the search icon when focused could work).
outline: noneon the input field should remove the default highlight behavior - The height and width of the doorhanger, individual items, and the 'recently used entries' element all differ from the spec
- Not sure if we're just using default fonts on each platform, but even if we are, the weights are pretty different (?)
- 'Manage Lockbox' text color is off
Filtered view (spec)
WIP on the left, spec on the right:
- Search and 'X' icons look different than spec
- Text should change from 'recently used' to 'N entry/entries found'
Hovered filtered view (spec)
WIP on the left, spec on the right:
- 'info' icon not shown on hover
- hover color seems off vs. spec
Entry Detail view (spec)
Again, WIP on the left, spec on the right:
Lot of work left to do here.
Go ahead and take another swing and re-request review when you'd like another pass
There was a problem hiding this comment.
Sorry for the delay in getting this reviewed, had some errors to resolve yesterday before I could get the build working.
This is looking good! @6a68 already beat me to most of the visual things I noticed, but I've included some additions below. There are also a couple items that we just recently updated and have gotten into Zeplin, apologies for adding work on those items.
Entry List - Default View
- For focus state, let's go with what @6a68 mentioned and have the color of the Search icon darker (current color in design, Grey 60), and when not focused, Gray 40.
- As mentioned previously, height, width of doorhanger as well as other elements are a bit off from spec
- Bottom button has been updated based on some research findings on other FF doorhangers with a gray action button at the bottom. (spec has been updated)
- Bottom button has been updated to "Open Lockbox" instead of "Manage Lockbox"
- Instruction text (Recently used entries. Select...) area background and text color has been updated due to conflicts with the entry item hover state (spec updated)
- Search placeholder text style seems off
- This has been updated to always show the info button on list items (not just on hover)
Entry List - Searching / Filtered
- As mentioned, icon is different and text needs changed to reflect # of matches
Entry List Item - Hover
- not currently showing the info button
- info button color change
- as mentioned, entry background is a bit off
- when hovering over info button, background is shown (ghost button)
Bottom button - Hover
- wasn't previously defined, but button now has darker state for when hovering
I also noticed that if you have your device in dark mode (and FF inherits that), we get a dark border on the doorhanger. Do we have any control over this?
I don't believe that the entry details is part of this PR, so not including comments regarding that. Let me know if that is not the case though
I also updated the issue this is attached to, to include links to all of these screens so you don't have to hunt them down in Zeplin. Sorry for not having that in there originally
|
@nickbrandt Thanks for the review I noticed the info icon downloaded from zeplin looks bad for some reason |
|
@nickbrandt @6a68 I believe all of your comments have now been addressed, I updated the pr description with screenshots also |
|
@meandavejustice this is looking really close, thanks for addressing those items. Just noticing a few remaining things, identified below: Entry List - Default
Entry List - Filtered/Searching
Entry List - Hover States
Entry List - Full View
Because the entry list items are tied to what you see in the full view, we are now seeing the info icons/buttons there as well. We actually don't need them there, can we update this for those to only be visible in the doorhanger? Thanks! |
|
@meandavejustice I'll try and look into the Info icon, it appears pretty close on my Mac, but does look a bit pixelated on your screenshot |
|
@meandavejustice Hey, when this is ready, remove the 'WIP' from the title, and re-request a review. Would be cool to fix the "merge master into branch" git noise, too. |
src/list/components/item-summary.js
Outdated
| @@ -74,6 +74,7 @@ export default function ItemSummary({className, id, title, username, verbose, | |||
| $length={trimmedUsername.length}> | |||
| <div data-name="subtitle" className={styles.subtitle}>no uSERNAMe</div> | |||
| </Localized> | |||
| <div className={styles.info}></div> | |||
There was a problem hiding this comment.
This inadvertently causes a breakage in test/unit/list/components/item-summary-test.js because more than one element matches div > div.
I think the most expedient way to fix htis is to change the merge classNames test to explicitly set an item id and then use it as data-item-id when find()ing the item.
meandavejustice
changed the title
meandavejustice
dismissed stale reviews from linuxwolf, nickbrandt, and jaredhirsch
Fixed up tests, and added test for new icon hide/show logic
There was a problem hiding this comment.
@meandavejustice this looks great! Just 1 small change I noticed still, as well as an additional item I had missed previously. After those are fixed we are good to go from a design standpoint. Thanks for all your work on this!
Filtered / Searching
- Search entered text needs updated to Gray 90 (#0C0C0D)
General Doorhanger
- The header (on both search and in entry details) is white, while the caret at the top of the doorhanger is the color of the doorhanger background. Can we update the caret to be white instead?
Thank you!
|
@meandavejustice I couldn't find anything wrong with the info icon we were exporting through Sketch, so I went ahead and downloaded it from the Photon site, and updated it to be the same color/sizing we were using (attached below). Let me know if this works out any better. It may be just some issue with exporting through Sketch and how that is getting translated (though that would be strange as other icons are working this way), but if you still have issues perhaps just try using the one straight from the Photon site and through CSS update the color/size. Let me know how it turns out! |
|
Thanks @nickbrandt just pushed an update, your version of the info icon took care of it on my end. |
|
@meandavejustice the caret looks good as well as the updated info icon 👍 Still seeing #737373 for the Search text. Looks like perhaps the empty state text got updated instead?
|
|
@nickbrandt Sorry, it should be actually correct now! |
|
@meandavejustice no worries, looks good now. Thanks!
|
There was a problem hiding this comment.
r+
Code wise this looks great; thanks @meandavejustice ! I'll let @nickbrandt take another glance to be sure visuals match.
Fixes #12
Screenshots or Videos
To Do