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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

馃枍Fix date picker drop shadow cutoff in static mode #15412

Merged
merged 1 commit into from May 18, 2018

Conversation

cvializ
Copy link
Contributor

@cvializ cvializ commented May 17, 2018

Also adds slight background color. Feedback welcome.

The drop shadow still appears in overlay mode, but in static mode there is no way to prevent cutting off the shadow inside the overflow:hidden amp element container with layout="{fixed,fixed-height,responsive,etc}".

before after
screen shot 2018-05-17 at 15 03 03 screen shot 2018-05-17 at 15 02 03

@nainar
Copy link
Contributor

nainar commented May 18, 2018

Could we potentially do the following:

  • On an inner container (your date picker in this case) add a border and then
  • Add a parent element to the date picker (the inner container) with the padding on that element being the same width as the shadow you want and then having that parent element have an inward box shadow (using inset)?

@cvializ
Copy link
Contributor Author

cvializ commented May 18, 2018

There's a light 1px border in the screenshot. Is that enough border for your first option?

I think for the static picker removing the box-shadow is best. Abby and I chatted and settled on the fact that drop shadow implies that the picker is floating on top of the rest of the content on the page, so it makes sense to only have the drop shadow for the overlay mode.

The problem with adding padding for the shadow is that the left edge of the picker will be shifted a couple of pixels and won't be aligned with the rest of the content above and below it in the layout.

@nainar
Copy link
Contributor

nainar commented May 18, 2018

My suggestion was to split up into two steps not two options. (Edited the suggestion to make that clearer.) But yes if you are dropping the idea of adding a shadow in the static option, then this conversation is moot.

@cvializ
Copy link
Contributor Author

cvializ commented May 18, 2018

Ah I see what you meant, thanks for explaining that. I haven't seen an inset box-shadow used for a drop shadow before, but it could be a good idea for if we encounter a similar decision point again. But yes, we are dropping the shadow in the static mode for now.

Copy link
Contributor

@nainar nainar left a comment

Choose a reason for hiding this comment

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

Should have approved with my last comment.

@cvializ cvializ merged commit 465205a into ampproject:master May 18, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants