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

Framework: Update search component (and instances) to use gridicons #1358

Merged
merged 5 commits into from
Dec 11, 2015

Conversation

kellychoffman
Copy link
Member

This is the first step in cleaning up all the searching instances: removing noticons for gridicons.

Section nav search:
(My Sites > Blog posts for example)
section nav

Menu search
menu

Author selector:
(Multi author site editor)
author

Category search:
(Category accordion in editor)
categories

Location search:
(More options accordion in editor)
location

(will squash before merging)

@kellychoffman kellychoffman added Framework [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. [Status] Needs Design Review Add this when you'd like to get a review / feedback from the Design team on your PR labels Dec 8, 2015
@kellychoffman kellychoffman self-assigned this Dec 8, 2015
@folletto
Copy link
Contributor

folletto commented Dec 8, 2015

Keep an eye open for wonkiness for the first one, we had troubles in the past and the component should be refactored. For reference see #109.

@folletto
Copy link
Contributor

folletto commented Dec 8, 2015

Checked:

  • My Sites → Blog Posts ❓
  • My Sites → Menus → Post Search ✅
  • My Sites → Themes ❓
  • Editor → Author Selector ❓
  • Editor → Category Search ❓
  • Editor → Location Search ❓

(1) My Sites → Blog Posts — the icon is misaligned (too high) when the dropdown is shown:

screen shot 2015-12-08 at 12 14 48

(2) Editor → Author Selector — Alignment seems off. A top: 9px and left: -3px seems aligning it to me (Chrome).

screen shot 2015-12-08 at 12 22 27

(3) Editor → Category Search — Alignment is ok, seems slightly better with left: -1px (instead of 0px).

(4) Editor → Location Search — Alignment seems slightly off. Try maybe left: 1px and top: 8px.

(5) My Sites → Themes — Top alignment slightly off. I'd go for 1px higher.

(6) All mobile views seem misaligned:

Themes:
screen shot 2015-12-08 at 12 28 04

Posts:
screen shot 2015-12-08 at 12 29 37

@kellychoffman kellychoffman added [Status] Needs Author Reply and removed [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. labels Dec 8, 2015
@kellychoffman
Copy link
Member Author

Hmm, I'm wondering if something didn't sync... All my screenshots were taken in Chrome. I'll double check and fix the remaining issues. Thanks for the solid review.

@rickybanister
Copy link

I fixed the mobile alignment, but we should fix the failing test before merging this

@rickybanister rickybanister force-pushed the update/search-gridicon branch from 84b06ec to c5a7f47 Compare December 8, 2015 19:30
@rickybanister
Copy link

I fixed the alignment of several other instances where there had been a hardcoded top defined.

Instead of maintained the top: 50% style and adjusted any custom margin-top to reflect 1/2 the height of the icon (if it was custom sized).

I verified on:

  • site-picker
  • section-nav: mobile-width
  • section-nav: with dropdown
  • section-nav: desktop-width
  • themes
  • new post: site-selector
  • editor: author-selector
  • editor: location
  • editor: category
  • devdocs

kellychoffman and others added 2 commits December 8, 2015 13:49
search open and close icons: replace all other instances of noticon references

search: alignment

multi author: align
…in the search component so that it is always vertically centered regardless of the height of the wrapping card

Search: fixed a regression in the modal site-selector search due to my previous commit which made search vertically centered (assuming a 24px icon)

Search: icon is now positioned correctly in every instance I could find, this includes the close icon

Search: missed one in the site selector close icon
@kellychoffman kellychoffman force-pushed the update/search-gridicon branch from c5a7f47 to 85b24bc Compare December 8, 2015 19:58
@kellychoffman kellychoffman added [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. and removed [Status] Needs Author Reply labels Dec 8, 2015
@kellychoffman
Copy link
Member Author

👍 Thanks @rickybanister. Looks good. Rebased to squash the commits.

@rickybanister
Copy link

Just to remind us, we should probably try to (in a new PR) make the gridicons be 18px and margin-top: -9px where necessary for sharper icons.

@timmyc
Copy link
Contributor

timmyc commented Dec 9, 2015

@kellychoffman tests should be fixed up with a24b1b1

@kellychoffman kellychoffman added [Status] Needs Author Reply and removed [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. labels Dec 10, 2015
@kellychoffman
Copy link
Member Author

Just to remind us, we should probably try to (in a new PR) make the gridicons be 18px and margin-top: -9px where necessary for sharper icons.

Updated in here instead of making a new PR d26554b

tests should be fixed up with a24b1b1

Thanks! I'm still seeing errors :/

@kellychoffman kellychoffman added [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. and removed [Status] Needs Author Reply labels Dec 10, 2015
@kellychoffman
Copy link
Member Author

Checks are passing now. Thanks @timmyc.

@MichaelArestad
Copy link
Contributor

Not sure if related, but go to the posts view on a site and search for something. Then hit the X to cancel a search. Nothing happens.

@kellychoffman
Copy link
Member Author

@MichaelArestad What browser? Can't seem to replicate.

@MichaelArestad
Copy link
Contributor

@kellychoffman Chrome Mac

Tried clearing cache, rebuilding, etc. Definitely up to date on this branch.

Video: https://cloudup.com/cedQ2GRIGVT

@timmyc
Copy link
Contributor

timmyc commented Dec 11, 2015

I just hit the same issue @MichaelArestad is showing on a different branch - so the issue is likely in master too. I'd say create a new issue for that specifically and not address it in this PR.

@kellychoffman
Copy link
Member Author

Thanks for the video. I thought you were referring to the close button not clearing the search query out.

Just tested, and I'm not seeing this in Staging.

@timmyc
Copy link
Contributor

timmyc commented Dec 11, 2015

@kellychoffman for me I had to toggle around a bit between the various filters first, drafts, trash etc. Then went back to published and opened up the search utility.

@kellychoffman
Copy link
Member Author

You're right. We can address in another PR.

All good here?

@timmyc
Copy link
Contributor

timmyc commented Dec 11, 2015

LGTM

@timmyc timmyc added [Status] Ready to Merge and removed [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. labels Dec 11, 2015
kellychoffman added a commit that referenced this pull request Dec 11, 2015
Framework: Update search component (and instances) to use gridicons
@kellychoffman kellychoffman merged commit 25d7ed6 into master Dec 11, 2015
@kellychoffman kellychoffman deleted the update/search-gridicon branch December 11, 2015 19:02
@lancewillett lancewillett removed the [Status] Needs Design Review Add this when you'd like to get a review / feedback from the Design team on your PR label Oct 19, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants