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

32 manage page list styles #76

Merged

Conversation

@jaredhirsch
Copy link
Contributor

@jaredhirsch jaredhirsch commented Feb 8, 2019

Fixes #32

Testing and Review Notes

Visual updates based on the zeplin mocks linked in #32.

This PR should only impact the management page list view; watch out for unexpected changes to the panel list view.

Questions for dev:

  • commit e09991e: I'm not crazy about the amount of panel vs page special case code in here, but it works; suggestions welcome.
  • Also in this same commit, I went with the positioned span approach for the right chevron shown in the list items, but based on the use of pseudoelements for list separators (which I adopted for the blue left-side border in commit b27aee8), it kinda seems like an ::after element would be a better option for both the right chevron and the info icon (in the panel list item view). However, item-summary already has the top/bottom border pseudoelements, and I wasn't sure whether it would be a net gain to add yet another wrapper element vs. just rolling with the current positioned span.

Question for UX:

I wasn't clear on the differences between hover, focus, and active states, based on the zeplin mocks. I'm particularly curious if these three states look correct for the list item, the search field, and the '+' button.

Screenshots or Videos

I'm going to skip these, to encourage reviewers to actually exercise the code a bit (afraid I'm missing some edge cases)...but if this seems a little too lazy on my part, I can add side-by-side screenshots for comparison purposes 🙃

@jaredhirsch jaredhirsch requested a review from mozilla-lockwise/desktop-engineering as a code owner Feb 8, 2019
@ghost ghost assigned jaredhirsch Feb 8, 2019
@ghost ghost added the in progress label Feb 8, 2019
@jaredhirsch jaredhirsch requested a review from mozilla-lockwise/ux Feb 8, 2019
Copy link

@nickbrandt nickbrandt left a comment

Overall, this is fantastic! Everything is just about spot on, with just a couple small nits (and a mistake on my end).

Main List View

screen shot 2019-02-11 at 4 18 25 pm

  • The only thing I'm seeing off, is the sidebar background color. We deviated here from Photon slightly, in order to have contrast with some of the other grays. Color used: #F2F2F4
  • Also, I missed removing the chevron on the active entry list item on certain screens (one being the one that was linked in your issue. Sorry!). Can we have that so whichever item is currently selected/active, that the chevron disappears? (now updated on zeplin)

Hover States

screen shot 2019-02-11 at 4 17 55 pm

  • You nailed it! Apologies, we had those defined in Sketch but not uploaded anywhere on Zeplin.

Search (I think this is included in your PR)

screen shot 2019-02-11 at 4 17 25 pm

  • The only thing off here is the Search placeholder text color. We have this as Gray 60 (#4a4a4f) for accessibility/color contrast purposes.

Nice work @6a68 ! 👍

@jaredhirsch
Copy link
Contributor Author

@jaredhirsch jaredhirsch commented Feb 12, 2019

Thanks for the thorough review, @nickbrandt! 💯

Copy link
Contributor

@linuxwolf linuxwolf left a comment

r+

Code-wise, I think this looks great. Pending the tweaks @nickbrandt asked for, it's good to :shipit:

@linuxwolf
Copy link
Contributor

@linuxwolf linuxwolf commented Feb 19, 2019

@nickbrandt @6a68 Looking over the existing issues, I think updates to the search bar are to be addressed via #36.

@nickbrandt
Copy link

@nickbrandt nickbrandt commented Feb 19, 2019

@linuxwolf sounds good!

@jaredhirsch jaredhirsch force-pushed the jaredhirsch:32-manage-page-list-styles branch from 6d10cab to e5bb47d Feb 21, 2019
@jaredhirsch jaredhirsch dismissed nickbrandt’s stale review Feb 21, 2019

Made the requested changes 👍

@jaredhirsch jaredhirsch merged commit 2cb7ab4 into mozilla-lockwise:master Feb 21, 2019
4 checks passed
4 checks passed
@wip
WIP Ready for review
Details
@codecov
codecov/patch 100% of diff hit (target 50%)
Details
@codecov
codecov/project 95.9% (+<.1%) compared to 10feb59
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@jaredhirsch jaredhirsch deleted the jaredhirsch:32-manage-page-list-styles branch Feb 21, 2019
@ghost ghost removed the in progress label Feb 21, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants