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

Media Library: fix elevation visual bug in search icon #11836

Merged
merged 1 commit into from
Mar 8, 2017

Conversation

retrofox
Copy link
Contributor

@retrofox retrofox commented Mar 7, 2017

IMPORTANT


Testing:

  1. Go to the Media Library: http://calypso.localhost:3000/media/<site>. You should have enough media files to be able to make scrolling up on the media page.
  2. Pay attention to the overlapping of the search-icon and the plan-storage components with the filters bar of the media library.

@matticbot
Copy link
Contributor

@matticbot matticbot added the [Size] S Small sized issue label Mar 7, 2017
@retrofox retrofox requested review from iamtakashi and gwwar March 7, 2017 15:31
@retrofox retrofox added [Feature] Media The media screen in Calypso, general media management, or integration with third party media. [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. labels Mar 7, 2017
@iamtakashi
Copy link
Contributor

Design wise 👍

@matticbot
Copy link
Contributor

@retrofox This PR needs a rebase

@@ -188,6 +188,9 @@ $z-layers: (
'.media-library__list-item-spinner': 20,
'.media-library__list-item-edit': 20
),
'.media-library': (
'.section-nav .search.is-expanded-to-container': 20,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just fyi that you can use this Chrome extension to figure out the parent context https://chrome.google.com/webstore/detail/z-context/jigamimbjojkdgnlldajknogfgncplbh

So a few things here:

  1. The parent stacking context is .section-nav to make this more explicit for the media library I'd recommend using:
'.media-library .section-nav': ( '.search.is-expanded-to-container' : 1,
  1. Why is this value 20? It doesn't appear like we need to beat any other items in the current stacking context so I think 1 works here just fine. Let me know if you had some corner cases in mind.

As an aside, we also have a z-index on the search icon, but it does nothing since the position is static.
screen shot 2017-03-08 at 10 37 33 am

@gwwar
Copy link
Contributor

gwwar commented Mar 8, 2017

Let's rebase this one @retrofox

@retrofox retrofox force-pushed the fix/media-search-bar-visual-bug branch from 46f3a27 to f123af6 Compare March 8, 2017 20:21
@matticbot matticbot added the [Size] S Small sized issue label Mar 8, 2017
@retrofox retrofox force-pushed the fix/media-search-bar-visual-bug branch from f123af6 to 10d4f43 Compare March 8, 2017 20:46
@matticbot matticbot added the [Size] S Small sized issue label Mar 8, 2017
@retrofox
Copy link
Contributor Author

retrofox commented Mar 8, 2017

Just fyi that you can use this Chrome extension to figure out the parent context https://chrome.google.com/webstore/detail/z-context/jigamimbjojkdgnlldajknogfgncplbh

awesome

The parent stacking context is .section-nav to make this more explicit for the media library I'd recommend using ...

👍

Why is this value 20?

I tested with values less than 20 and it overlapped the media library header. Tested with 1 and it works ok.

As an aside, we also have a z-index on the search icon, but it does nothing since the position is static.

yeah, right. I wonder why we are keeing this value.

rebased and tested

Copy link
Contributor

@gwwar gwwar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @retrofox Let's :shipit:

@retrofox retrofox merged this pull request into try/media-section-v2 Mar 8, 2017
@retrofox retrofox deleted the fix/media-search-bar-visual-bug branch March 8, 2017 21:26
@matticbot matticbot removed the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label Mar 8, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Media The media screen in Calypso, general media management, or integration with third party media. [Size] S Small sized issue [Status] Needs Rebase
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants